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

Jump to bottom

proposal: crypto/tls: allow configurability of supported Signature Hash Algorithms #28660

Closed
e3b0c442 opened this issue Nov 8, 2018 · 6 comments
Closed

proposal: crypto/tls: allow configurability of supported Signature Hash Algorithms #28660

e3b0c442 opened this issue Nov 8, 2018 · 6 comments
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Go1.14

Comments

@e3b0c442
Copy link

e3b0c442 commented Nov 8, 2018

Go up to and including the current version (at this writing, 1.11.2) hardcodes the list of supported algorithms for the TLS 1.2 Signature Algorithms extension. Concurrently, crypto/tls also allows the use of custom signers which may have their own limitations not addressed by the hardcoded list of supported algorithms.

One real-world example of this is a custom signer for a TLS client that uses a hardware backend such as a Trusted Platform Module. TPM 1.2 modules can only support SHA1, and while TPM 2.0 modules can support SHA512, they are only required under the current spec to support SHA1 and SHA256. Depending on the list of algorithms provided by the remote party, the Go implementation may choose a 384-bit or 512-bit algorithm that is not supported by the backing hardware module, causing the handshake to fail.

In order to address this, I propose adding a configuration option to tls.Config to allow a custom list of supported signature algorithms to be provided, which will allow custom signer implementations to express the algorithms they support.
@bradfitz bradfitz added FeatureRequest Proposal-Crypto Proposal related to crypto packages or other security issues labels Nov 8, 2018
@bradfitz bradfitz changed the title crypto/tls: allow configurability of supported Signature Hash Algorithms proposal: crypto/tls: allow configurability of supported Signature Hash Algorithms Nov 8, 2018
@gopherbot gopherbot added this to the Proposal milestone Nov 8, 2018
@FiloSottile
Copy link
Contributor

This sounds like it would be more cleanly addressed by an interface upgrade on crypto.Signer allowing it to communicate which ones it supports, than by yet another tls.Config switch. Moreover, this is a property of the selected certificate, not of the system as a whole (forcing the use of GetConfigForClient.

Happy to consider this for Go 1.13. Go 1.12 is now in feature freeze.

@e3b0c442
Copy link
Author

e3b0c442 commented Nov 9, 2018

While I understand the line of thinking, I'm not sure tying this directly to crypto.Signer is the best option.

In order to query the crypto.Signer, there would need to exist some set of values to define the algorithms the crypto.Signer supports. While such a list exists in crypto/tls, I see two problems co-opting that for the crypto.Signer:

  1. crypto.Signer is more generalized and can be used in contexts not related to TLS, so one would not want crypto.Signer to refer to crypto/tls for a list of allowable values.

  2. Given that crypto.Signer can be an arbitrary implementation, I'm not sure it is possible or advisable to try to enumerate the parameters needed for the TLS implementation to determine a list of supported algorithms.

Given these points, I believe it makes the most sense to define this selection in the crypto/tls package somehow. That said, there is another option that satisfies keeping the selection in crypto/tls while not further complicating tls.Config: adding the field to tls.Certificate. This ties the algorithm selection to the crypto.Signer while not encumbering crypto.Signer with TLS knowledge, while at the same not adding additional fields to tls.Config and recognizing that the limitations are with the private key signer and not with the client itself.

@efb2
Copy link

efb2 commented Jun 3, 2019

Curious, has this been looked at since the proposal? It is a practical limitation since many TPM2 implementations do not implement SHA384/512.
It doesn't seem like it, but are there any workarounds aside from modifying crypto/tls.

@FiloSottile
Copy link
Contributor

I like the idea of adding SupportedSignatureAlgorithms to tls.Certificate. The constants are already exposed, so it should be fairly unobtrusive, and it also lets TLS 1.2 clients opt out of RSA-PSS.

Approving for Go 1.14.

@FiloSottile FiloSottile modified the milestones: Proposal, Go1.14 Jun 4, 2019
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 4, 2019
@FiloSottile FiloSottile self-assigned this Jun 4, 2019
@FiloSottile FiloSottile mentioned this issue Jun 4, 2019
Closed
@tmthrgd tmthrgd mentioned this issue Jun 4, 2019
Closed
@FiloSottile FiloSottile mentioned this issue Jul 22, 2019
Closed
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@gopherbot
Copy link

Change https://golang.org/cl/205062 mentions this issue: crypto/tls: implement Certificate.SupportedSignatureAlgorithms

@gopherbot
Copy link

Change https://golang.org/cl/205063 mentions this issue: crypto/tls: re-enable RSA-PSS in TLS 1.2 again

gopherbot pushed a commit that referenced this issue Nov 12, 2019
@FiloSottile
crypto/tls: re-enable RSA-PSS in TLS 1.2 again
52a5bf4 
TLS 1.3, which requires RSA-PSS, is now enabled without a GODEBUG
opt-out, and with the introduction of
Certificate.SupportedSignatureAlgorithms (#28660) there is a
programmatic way to avoid RSA-PSS (disable TLS 1.3 with MaxVersion and
use that field to specify only PKCS#1 v1.5 SignatureSchemes).

This effectively reverts 0b3a57b,
although following CL 205061 all of the signing-side logic is
conveniently centralized in signatureSchemesForCertificate.

Fixes #32425

Change-Id: I7c9a8893bb5d518d86eae7db82612b9b2cd257d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/205063
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@golang golang locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
6 participants
@bradfitz @rsc @FiloSottile @gopherbot @e3b0c442 @efb2