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

Support Windows SCEP request certificates on Go 1.23 #28

Merged
merged 13 commits into from
Sep 11, 2024

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented Sep 9, 2024

Notable changes:

  • Add SetFallbackLegacyX509CertificateParserEnabled to enable parsing using the "legacy" crypto/x509 certificate parser upon failure to parse using the version the code is compiled with.
  • Introduced the legacyx509 package, which contains parts of the Go crypto/x509 stdlib at 1.23, specifically the parts required to parse X509 certificates.
    • Removed support for parsing policy OIDs to prevent having to rely on a copy of x509.Certificate
    • Removed support for parsing X25519 public keys
    • Replaced some (recent) stdlib functions with copies, so that they work in older Go versions
  • Removed Go 1.12 and Go 1.13 from CI. There's an open issue for running on more recent Go versions, but there's more things to fix related to legacy algorithms in that case: Update CI #13.

We can likely add the removed parts back when the package as a whole requires a new Go version.

@hslatman hslatman marked this pull request as ready for review September 10, 2024 12:32
@hslatman hslatman requested review from a team, azazeal and dopey September 10, 2024 12:46
@@ -0,0 +1,45 @@
//go:build go1.23
Copy link

@azazeal azazeal Sep 10, 2024

Choose a reason for hiding this comment

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

Considering the build matrix in ci.yml would this file every be executed during our builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in our CI, atm, no. But it works on my machine 😄

Copy link

Choose a reason for hiding this comment

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

Sure but maybe expand the matrix to include these later Go versions that we are interested in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, but that's why I called out #13 in the OP. There are some more things to fix before we can do so.

Copy link

Choose a reason for hiding this comment

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

Maybe just add Go 1.23 then? Or it will break the build if you add it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try, but my guess is it breaks, yes

Copy link
Member Author

Choose a reason for hiding this comment

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

🎉

pkcs7.go Outdated
// if it fails on the above case, it'll retry parsing the certificates using a
// copy of the crypto/x509 package based on Go 1.23, but skips checking the
// authority key identifier extension being critical or not.
var EnableFallbackLegacyX509CertificateParser bool
Copy link

Choose a reason for hiding this comment

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

Perhaps a function that sets/unsets an atomic bit might be better here, instead of a global.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hslatman hslatman mentioned this pull request Sep 10, 2024
2 tasks
p7, err = Parse(data)
if err != nil {
t.Errorf("failed parsing SCEP request data with legacy X509 certificate parser enabled: %v", err)
}
EnableFallbackLegacyX509CertificateParser(false)
SetFallbackLegacyX509CertificateParserEnabled(false)
Copy link

@azazeal azazeal Sep 11, 2024

Choose a reason for hiding this comment

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

If an error happens, you won't switch it back for the other tests. Perhaps do this in cleanup.

Scratch the above. It's Errorf & not Fatalf.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too fast: 46c9f76 😅

azazeal
azazeal previously approved these changes Sep 11, 2024
@hslatman hslatman merged commit b1cae62 into main Sep 11, 2024
14 checks passed
@hslatman hslatman deleted the herman/support-windows-scep-request-certificates branch September 11, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants