Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crl provider: Static and FileWatcher provider implementations #6670

Merged
merged 63 commits into from
Oct 31, 2023
Merged

crl provider: Static and FileWatcher provider implementations #6670

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
978cb44
rename certificateListExt to CRL
gtcooke94 Jun 29, 2023
7d032e0
CRLProvider file
gtcooke94 Jun 29, 2023
cdbc298
Add CRLProvider to RevocationConfig
gtcooke94 Jun 29, 2023
95991d8
Beginning refactor of CRL handling
gtcooke94 Jun 29, 2023
32e3158
Shell of StaticCRLProvider
gtcooke94 Jun 29, 2023
00de36e
basic static crl provider test
gtcooke94 Jun 29, 2023
8033cab
use loadCRL helper
gtcooke94 Jun 30, 2023
338a7f4
refactor of CRL loading
gtcooke94 Jun 30, 2023
d1f63fe
Table tests
gtcooke94 Jun 30, 2023
01afa97
Table tests
gtcooke94 Jun 30, 2023
401eb79
Add tests with Static CRL provider
gtcooke94 Jun 30, 2023
c88d12d
New certs to be used for CRL tests. Added test for passing and failin…
erm-g Sep 26, 2023
1feaae3
Main functionality of File Watcher (Directory) CRL provider
erm-g Sep 28, 2023
a9a84f1
Refactor async go routine, validate() func, add unit tests
erm-g Sep 29, 2023
5a0acad
Custom error callback, related unit tests
erm-g Sep 29, 2023
f3c830b
Error callback test improvement
erm-g Oct 1, 2023
4ea1b34
Comments for StaticCRLProvider
erm-g Oct 1, 2023
aeebd4e
Comments for public API
erm-g Oct 2, 2023
735ac20
go mod tidy
erm-g Oct 2, 2023
5c76a60
Comments for tests
erm-g Oct 2, 2023
0bc7757
Fix vet errors
erm-g Oct 2, 2023
f844c8c
Change Static provider behavior to match C Core, address other PR com…
erm-g Oct 3, 2023
a4da85e
Data race fix
erm-g Oct 4, 2023
c3ba07e
Test helper fn change
gtcooke94 Oct 4, 2023
ffe5c34
Address PR comments
erm-g Oct 8, 2023
6d28181
Address PR comments (part 2)
erm-g Oct 9, 2023
7814373
Migration from context to channel for controlling crl reloading gorou…
erm-g Oct 9, 2023
1a46b65
Align in-memory CRL updates during directory scan to C++ behavior
erm-g Oct 10, 2023
d7f1555
Improve comments for ScanCRLDirectory
erm-g Oct 10, 2023
2f1935d
Base test case for Scan CRL Directory file manipulations
erm-g Oct 10, 2023
0a7b086
full set of cases for CRL directory content manipulation
erm-g Oct 10, 2023
8d05f28
Add comment for table test structure
erm-g Oct 10, 2023
ccbf7f6
Fix for go.mod and go.sum
erm-g Oct 11, 2023
99ecab0
Empty directoru workaround
erm-g Oct 11, 2023
9e5a70d
Delete deprecated crl functionality
erm-g Oct 11, 2023
5643760
Restoring deprecated crl files
erm-g Oct 11, 2023
8898959
Fit to grpctest.Tester pattern
erm-g Oct 12, 2023
b16af8b
Update readme for crl provider tests
erm-g Oct 16, 2023
21f4301
Address PR comments
erm-g Oct 16, 2023
51b42aa
Revert "Restoring deprecated crl files"
gtcooke94 Oct 16, 2023
f0c1ca4
Merge remote-tracking branch 'upstream/master' into CrlTemp
gtcooke94 Oct 16, 2023
08188d1
Revert "Resolve conflicts with upstream - deletion of deprecated crl"
erm-g Oct 16, 2023
9b8d07e
Update link for gRFC proposal
erm-g Oct 19, 2023
ad15e23
Address PR comments
erm-g Oct 19, 2023
8e02546
Address PR comments part 1
erm-g Oct 20, 2023
e6a690d
Address PR comments part 2
erm-g Oct 20, 2023
340757d
Address PR comments part 3
erm-g Oct 20, 2023
1e4c5ac
Fix for go.mod and go.sum
erm-g Oct 20, 2023
f654d18
Fix comment typo
erm-g Oct 23, 2023
53d6b05
Fix for gRFC tag
erm-g Oct 23, 2023
1f398eb
Add more details to CRL api godoc comments.
erm-g Oct 23, 2023
f3dcca1
Address PR comments
erm-g Oct 24, 2023
131e6e7
Address PR comments
erm-g Oct 24, 2023
bc14ea8
Delete crl_deprecated.go and crl_deprecated_test.go
erm-g Oct 24, 2023
96bf905
Delete testdate/crl/provider/filewatcher directory and .gitignore und…
erm-g Oct 24, 2023
0ce6a2c
Race test fix
erm-g Oct 27, 2023
c57a08a
Address PR comments
erm-g Oct 27, 2023
4c53c56
Address PR comments
erm-g Oct 27, 2023
7fedab5
Refactor directory reloader test from checking size of crl map to que…
erm-g Oct 28, 2023
1025333
Add extra case for RefreshDuration config test
erm-g Oct 28, 2023
d9ba363
Update cpmment for table test structure
erm-g Oct 30, 2023
150e585
Unexport scan scanCRLDirectory, drop related mutex, update the comments
erm-g Oct 30, 2023
d7cf48f
Update API comments, clear tmp dir after the tests
erm-g Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Custom error callback, related unit tests
  • Loading branch information
@erm-g
erm-g committed Sep 29, 2023
commit 5a0acad848b23576bbdc955476359885a042f6fa
14 changes: 12 additions & 2 deletions security/advancedtls/crl_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ func (p *StaticCRLProvider) CRL(cert *x509.Certificate) (*CRL, error) {
}

type Options struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should name this FileWatcherOptions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively (and this is a go style thing so I'm not 100% sure) we could just eschew the Options struct and have the MakeFileWatcherCrlProvider just take these arguments directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use here credentials/tls/certprovider/pemfile/watcher.go as a pattern so both Options as a wrapping args pattern and as a name come from there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options are good, but advancedtls.Options is not the right name for something only used for the file watcher crl provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CRLDirectory string
RefreshDuration time.Duration
CRLDirectory string
RefreshDuration time.Duration
cRLReloadingFailedCallback func(err error)
}

// NewFileWatcherCRLProvider creates a new FileWatcherCRLProvider.
Expand Down Expand Up @@ -141,12 +142,18 @@ func (p *FileWatcherCRLProvider) scanCRLDirectory() {
dir, err := os.Open(p.opts.CRLDirectory)
if err != nil {
grpclogLogger.Errorf("Can't open CRLDirectory %v", p.opts.CRLDirectory, err)
if p.opts.cRLReloadingFailedCallback != nil {
p.opts.cRLReloadingFailedCallback(err)
}
}
defer dir.Close()

files, err := dir.ReadDir(0)
if err != nil {
grpclogLogger.Errorf("Can't access files under CRLDirectory %v", p.opts.CRLDirectory, err)
if p.opts.cRLReloadingFailedCallback != nil {
p.opts.cRLReloadingFailedCallback(err)
}
}

successCounter := 0
Expand All @@ -157,6 +164,9 @@ func (p *FileWatcherCRLProvider) scanCRLDirectory() {
if err != nil {
failCounter++
grpclogLogger.Warningf("Can't add CRL from file %v under CRLDirectory %v", filePath, p.opts.CRLDirectory, err)
if p.opts.cRLReloadingFailedCallback != nil {
p.opts.cRLReloadingFailedCallback(err)
}
continue
}
successCounter++
Expand Down
71 changes: 25 additions & 46 deletions security/advancedtls/crl_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import (
"google.golang.org/grpc/security/advancedtls/testdata"
)

const nonCRLFilesUnderCRLDirectory = 5
dfawley marked this conversation as resolved.
Show resolved Hide resolved

func TestStaticCRLProvider(t *testing.T) {
p := MakeStaticCRLProvider()
for i := 1; i <= 6; i++ {
Expand Down Expand Up @@ -89,62 +91,35 @@ func TestFileWatcherCRLProviderConfig(t *testing.T) {
t.Fatal("Unexpected error:", err)
}

customCallback := func(err error) {
fmt.Printf("Custom error message: %v", err)
}
regularProvider, err := MakeFileWatcherCRLProvider(Options{
CRLDirectory: testdata.Path("crl"),
RefreshDuration: 5 * time.Second,
CRLDirectory: testdata.Path("crl"),
RefreshDuration: 5 * time.Second,
cRLReloadingFailedCallback: customCallback,
})
if err != nil {
t.Fatal("Unexpected error while creating regular FileWatcherCRLProvider:", err)
}

regularProvider.scanCRLDirectory()
tests := []struct {
desc string
certs []*x509.Certificate
expectNoCRL bool
}{
{
desc: "Unrevoked chain",
certs: makeChain(t, testdata.Path("crl/unrevoked.pem")),
},
{
desc: "Revoked Intermediate chain",
certs: makeChain(t, testdata.Path("crl/revokedInt.pem")),
},
{
desc: "Revoked leaf chain",
certs: makeChain(t, testdata.Path("crl/revokedLeaf.pem")),
},
{
desc: "Chain with no CRL for issuer",
certs: makeChain(t, testdata.Path("client_cert_1.pem")),
expectNoCRL: true,
},
}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
for _, c := range tt.certs {
crl, err := regularProvider.CRL(c)
if err != nil {
t.Fatalf("Expected error fetch from provider: %v", err)
}
if crl == nil && !tt.expectNoCRL {
t.Fatalf("CRL is unexpectedly nil")
}
}
})
}
regularProvider.Close()
}

func TestFileWatcherCRLProvider(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test verifying the async refreshing goroutine works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, it'll require playing with the timers so I'm planning to have a separate PR for that (like e2e test)

p := MakeStaticCRLProvider()
for i := 1; i <= 6; i++ {
crl := loadCRL(t, testdata.Path(fmt.Sprintf("crl/%d.crl", i)))
p.AddCRL(crl)
// testdata.Path("crl") contains 5 non-crl files.
failedCRlsCounter := 0
customCallback := func(err error) {
failedCRlsCounter++
}

p, err := MakeFileWatcherCRLProvider(Options{
CRLDirectory: testdata.Path("crl"),
RefreshDuration: 5 * time.Second,
cRLReloadingFailedCallback: customCallback,
})
if err != nil {
t.Fatal("Unexpected error while creating FileWatcherCRLProvider:", err)
}
p.scanCRLDirectory()
tests := []struct {
desc string
certs []*x509.Certificate
Expand Down Expand Up @@ -182,4 +157,8 @@ func TestFileWatcherCRLProvider(t *testing.T) {
}
})
}
if failedCRlsCounter < nonCRLFilesUnderCRLDirectory {
t.Fatalf("Number of callback execution is smaller then number of non-CRL files: got %v, want at least %v", failedCRlsCounter, nonCRLFilesUnderCRLDirectory)
}
p.Close()
}