Fix cryptographic certificates for post go 1.19#3161
Closed
dperny wants to merge 1 commit into
Closed
Conversation
Member
|
Looks like generated files need an update (also golangci-lint failing; could be out of memory?) |
thaJeztah
reviewed
Jan 16, 2024
Comment on lines
+463
to
+471
| // defer close(actualErrChan) | ||
| require.Error(t, err) | ||
| err = <-actualErrChan | ||
| require.Error(t, err) | ||
| require.IsType(t, x509.UnknownAuthorityError{}, err) | ||
| require.ErrorAs(t, err, &x509.UnknownAuthorityError{}) | ||
| // require.IsType(t, x509.UnknownAuthorityError{}, err) | ||
|
|
||
| _, actualErrChan, err = tlsGRPCDial(tc.Context, l.Addr().String(), tcConfig.ClientTLSCreds) | ||
| defer close(actualErrChan) | ||
| // defer close(actualErrChan) |
Member
There was a problem hiding this comment.
Are these commented-out lines temporary? Or forgot to remove?
Collaborator
Author
|
Something is broken with the linter. Even on my local machine, running |
Member
|
Wondering if it needs settings tweaked; if it's a case of it running in a container and trying to spawn too many parallel things 🤔 I know I had to tweak GOMAXPROCS and some other things some years back for moby/moby |
Go 1.19 changed the way cryptographic certificates were verified, which broke a certain edge case of root CA rotation. This edge case is now disallowed. Signed-off-by: Drew Erny <derny@mirantis.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3161 +/- ##
==========================================
+ Coverage 57.28% 57.35% +0.07%
==========================================
Files 144 144
Lines 30598 30603 +5
==========================================
+ Hits 17528 17553 +25
+ Misses 11732 11709 -23
- Partials 1338 1341 +3 |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
Go 1.19 changed the way cryptographic certificates were verified, which broke a certain edge case of root CA rotation. This edge case is now disallowed.
- How I did it
As of go 1.19, the logic for certificate trust chain validation changed, and a chain including two certs with the same key will not validate. This case would usually occur when reissuing the same cert with a later expiration date. Because of this validation failure, our root rotation algorithm fails. While it might be possible to adjust the rotation procedure to accommodate such a cert change, it is somewhat of an edge case, and, more importantly, we do not currently possess the cryptographic expertise to safely make such a change. So, as a result, this operation is disallowed. The new root cert must have a new key.
- How to test it
Test changes within.