-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add versioning to dotnet-dev-certs #10908
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
Conversation
I got a very weird exception on the mac agents.
|
src/Tools/FirstRunCertGenerator/test/CertificateManagerTests.cs
Outdated
Show resolved
Hide resolved
@javiercn do you still want to take another look before I merge? |
@jkotalik Yes, I started but wasn't 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.
Looks good, but leaving it to @javiercn to sign-off ;)
@@ -171,10 +195,22 @@ public X509Certificate2 CreateAspNetCoreHttpsDevelopmentCertificate(DateTimeOffs | |||
pathLengthConstraint: 0, | |||
critical: true); | |||
|
|||
byte[] bytePayload; | |||
|
|||
if (AspNetHttpsCertificateVersion != 0) |
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.
This raises the question of if we need to support creating old cert versions. I don't think we do, at least not yet. The new cert should be compatible with old code (it just adds some EKUs that were technically required anyway). If we make a breaking change to the cert we could add this kind of logic back.
That would also mean AspNetHttpCertificateVersion
can be static readonly
.
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.
I doubt we need to, and even if we need to, we can work around it. I'd rather not over-engineer this. The only reason AspNetHttpCertificateVersion is public is for testing. I can make it internal.
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.
We don't need to support going back and I don't think we would ever need. The cert needs to be backwards compatible or we would have to use a different OID if we have to make breaking changes
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.
Looks good, but leaving it to @javiercn to sign-off ;)
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.
Looks good, but leaving it to @javiercn to sign-off ;)
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.
Looks good, but leaving it to @javiercn to sign-off ;)
Co-Authored-By: Andrew Stanton-Nurse <[email protected]>
… into jkotalik/devCert
@javiercn added a swanky fixture to cleanup certs on startup and shutdown (still need to clean up before each test). |
Fixes #9810
Also does a few cleanup items: