FIPS compliancy

[pavolloffay]

Feature Request

Related to #21047

This library depends on x/crypto which does not have FIPS validated crypto algorithms:

./vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_certificate_credential.go:19:	"golang.org/x/crypto/pkcs12"

Not FIPS compliant: pkcs12.ToPEM() performs a MAC check operation, for which it uses getSafeContents(), which calls verifyMac(), which calls pbkdf(), which is implemented in x/crypto, and thus not FIPS validated.

References:

• pkcs12.ToPEM(): https://cs.opensource.google/go/x/crypto/+/master:pkcs12/pkcs12.go;bpv=1;bpt=1;l=114
• getSafeContents(): https://cs.opensource.google/go/x/crypto/+/master:pkcs12/pkcs12.go;l=284;drc=b2d3a6a4b4d36521cd7f653879cf6981e7c5c340
• verifyMac(): https://cs.opensource.google/go/x/crypto/+/master:pkcs12/mac.go;drc=b2d3a6a4b4d36521cd7f653879cf6981e7c5c340;l=30
• pbkdf(): https://cs.opensource.google/go/x/crypto/+/master:pkcs12/pbkdf.go;drc=b2d3a6a4b4d36521cd7f653879cf6981e7c5c340;l=33

Would you accept a patch to fix this?

[jhendrixMSFT]

What would be the replacement for the current crypto module?

[pavolloffay]

I don't know exactly. I am not sure if there is an alternative in the main crypto package. If no then could we add a build tag to use the implementation from openssl?

[chlowell]

Only azidentity.ParseCertificates calls pkcs12.ToPEM (see here). This is a helper method for getting a cert and key to pass to NewClientCertificateCredential. That constructor takes the cert and key as types from the standard library's crypto module, so applications aren't required to call ParseCertificates. Is the pkcs12.ToPEM call a problem even if it's never executed?

[github-actions]

Hi @pavolloffay. Thank you for opening this issue and giving us the opportunity to assist. To help our team better understand your issue and the details of your scenario please provide a response to the question asked above or the information requested above. This will help us more accurately address your issue.

[github-actions]

Hi @pavolloffay, we're sending this friendly reminder because we haven't heard back from you in 7 days. We need more information about this issue to help address it. Please be sure to give us your input. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

[pavolloffay]

@chlowell I got this reply from our FIPS folks:

Not FIPS compliant: pkcs12.ToPEM() performs a MAC check operation, for which it uses getSafeContents(), which calls verifyMac(), which calls pbkdf(), which is implemented in x/crypto, and thus not FIPS validated.

[chlowell]

Sure, that makes sense. What I want to understand is whether this is a static analysis or a runtime problem. Our cert authentication implementation doesn't require applications to call pkcs12.ToPEM(), so we have a straightforward workaround if your application can comply by simply not calling that function. But if the fact that the module contains a line invoking pkcs12.ToPEM() makes your application noncompliant even if it never executes that line, that's a hard problem 😄

[github-actions]

Hi @pavolloffay. Thank you for opening this issue and giving us the opportunity to assist. To help our team better understand your issue and the details of your scenario please provide a response to the question asked above or the information requested above. This will help us more accurately address your issue.

[github-actions]

Hi @pavolloffay, we're sending this friendly reminder because we haven't heard back from you in 7 days. We need more information about this issue to help address it. Please be sure to give us your input. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

[pavolloffay]

This is supposed to be a runtime problem.

Are you saying that the app importing azure SDK will never cause invoking pkcs12.ToPEM() at runtime?

[chlowell]

Are you saying that the app importing azure SDK will never cause invoking pkcs12.ToPEM() at runtime?

Yes. azidentity invokes pkcs12.ToPEM() only when you call azidentity.ParseCertificates with a PKCS#12 (PFX) cert. That function exists to help apps get the []*x509.Certificate and crypto.PrivateKey for azidentity.NewClientCertificateCredential but you don't have to use it, the constructor parameters are simple standard library types you can get however you like.

This is supposed to be a runtime problem.

😅 Great! Then we have a couple straightforward workarounds:

• parse certs yourself i.e. don't call azidentity.ParseCertificates
• use only PEM format for certs

[github-actions]

Hi @pavolloffay. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

[pavolloffay]

This is great, thanks :)

Would it be possible to deprecate->remove azidentity.ParseCertificates so that we get rid of x/crypto import entirely from this package?

[jhendrixMSFT]

Removing it would be a breaking change and that bar is very high, so it's unlikely. In addition, others without FIPS requirements might find value in the helper.

[pavolloffay]

azidentity.ParseCertificates is also called from:

• azidentity.NewEnvironmentCredential
• azidentity.NewDefaultAzureCredential
• azidentity.NewOnBehalfOfCredentialWithCertificate

[chlowell]

You're quite right, NewEnvironmentCredential and NewDefaultAzureCredential call ParseCertificates when AZURE_CLIENT_CERTIFICATE_PATH is set, because they must parse the specified file. "Parse certs yourself" is still the best guidance: when you need to authenticate with a cert, parse it in your own code and call a constructor that takes []*x509.Certificate and crypto.PrivateKey i.e., NewClientCertificateCredential or NewOnBehalfOfCredentialWithCertificate.