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.

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
Show file tree
Hide file tree
Changes from 2 commits
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
3 changes: 1 addition & 2 deletions security/advancedtls/crl_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@ func (o *FileWatcherOptions) validate() error {
return fmt.Errorf("advancedtls: CRLDirectory %v is not readable: %v", o.CRLDirectory, err)
}
// Checks related to RefreshDuration.
if o.RefreshDuration <= 0 {
grpclogLogger.Infof("RefreshDuration is not set or negative: provided value %v, default value %v will be used.", o.RefreshDuration, defaultCRLRefreshDuration)
if o.RefreshDuration == 0 {
o.RefreshDuration = defaultCRLRefreshDuration
}
if o.RefreshDuration < minCRLRefreshDuration {
Expand Down
10 changes: 9 additions & 1 deletion security/advancedtls/crl_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"io"
"os"
"path/filepath"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -137,9 +138,13 @@ func (s) TestFileWatcherCRLProviderConfig(t *testing.T) {
// that it’s correctly processed. Additionally, we also check if number of
// invocations of custom callback is correct.
func (s) TestFileWatcherCRLProvider(t *testing.T) {
t.Parallel()
const nonCRLFilesUnderCRLDirectory = 5
nonCRLFilesSet := make(map[string]struct{})
var nonCRLMutex sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

So can you delete this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there is no need to protect this local to the test map anymore. We do the read once provider's goroutine is done

customCallback := func(err error) {
nonCRLMutex.Lock()
defer nonCRLMutex.Unlock()
nonCRLFilesSet[err.Error()] = struct{}{}
}
p, err := NewFileWatcherCRLProvider(FileWatcherOptions{
Expand Down Expand Up @@ -191,10 +196,11 @@ func (s) TestFileWatcherCRLProvider(t *testing.T) {
}
})
}
p.Close()
if len(nonCRLFilesSet) < nonCRLFilesUnderCRLDirectory {
t.Fatalf("Number of callback executions: got %v, want %v", len(nonCRLFilesSet), nonCRLFilesUnderCRLDirectory)
}
p.Close()

}

// TestFileWatcherCRLProviderDirectoryScan tests how FileWatcherCRLProvider
Expand Down Expand Up @@ -258,6 +264,8 @@ func (s) TestFileWatcherCRLProviderDirectoryScan(t *testing.T) {
t.Run(tt.desc, func(t *testing.T) {
copyFiles(sourcePath, targetPath, tt.fileNames, t)
p.ScanCRLDirectory()
p.scanMutex.Lock()
defer p.scanMutex.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

Anything similar here? We really shouldn't need mutexes while testing conditions, or else they might be flaky if we lose a race.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I'm not sure - I need the same instance of Provider for all the table tests, and I need to make the check cmp.Diff(len(p.crls), tt.expectedEntries) before the next test case scan, that's why I use mutex from p.ScanCRLDirectory

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, you're taking the mutex from the thing you're testing.

Can you call p.CRL() instead of directly accessing p.crls then?

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 think it's doable - I can provide a list of filenames to be scanned ([]string{"2.crl", "3.crl"} and list of CRLs to be found in the map by calling p.CRL()
Some downside - the current approach ensures the map contains 'exactly x' entries (len(p.crls)) but the new one will be about 'at least x' entries. Is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, CRL() takes a parameter and you'd need to call it multiple times.

This is OK, then, though I'd prefer stricter validation, i.e. checking the contents of the map, or at least the keys, and not just the length.

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 did the refactoring to the stricter validation and I think it looks better now (plus no more calling mutexes inside tests)

if diff := cmp.Diff(len(p.crls), tt.expectedEntries); diff != "" {
t.Errorf("Expected number of entries in the map do not match\ndiff (-got +want):\n%s", diff)
}
Expand Down
Loading