-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update crypto + bugfix in x509KeyPairProvider #122
Update crypto + bugfix in x509KeyPairProvider #122
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 153 of 153 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/grpc/golden.topo
line 1 at r1 (raw file):
---
I think this file is duplicated and not necessary here.
go/pkg/trust/x509KeyPairProvider.go
line 67 at r1 (raw file):
continue } if bestChain != nil && bestExpiry.Before(expiry) {
all good, but can we extend the UT for LoadX509KeyPair
so that it covers this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my side, the test for go/pkg/ca/renewal
fails (needs updated testdata).
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained
go/pkg/cs/drkey/grpc/golden.topo
line 1 at r1 (raw file):
Previously, juagargi (Juan A. Garcia Pardo) wrote…
I think this file is duplicated and not necessary here.
Done.
go/pkg/trust/x509KeyPairProvider.go
line 67 at r1 (raw file):
Previously, juagargi (Juan A. Garcia Pardo) wrote…
all good, but can we extend the UT for
LoadX509KeyPair
so that it covers this as well?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 41 of 41 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
This PR resolves #121 updating the crypto material using
go test ./... -update-non-deterministic
. In the future we might want to automatically regenerate the crypto material (e.g. https://github.com/scionproto/scion/blob/39b8fd4acbc2773e04e022bd66cf7cffae1f48a7/private/trust/x509_provider_test.go#L45).Additionally, this PR includes a bugfix for the x509KepPairProvider, in order to choose the certificate chain the expires the latest.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)