proposal: crypto/dsa: Implement crypto.Signer

[Merovius]

I would like to add an implementation of crypto.Signer to *dsa.PrivateKey. It's simple to do, but a) is an API change and b) creates a new dependency crypto/dsa -> encoding/asn1, so I'm going through the proposal process.

I am writing a package to use gpgagent for private-key operations. This proposal isn't directly related to that, but having the Signer interface provides a nice complement to other existing public key ciphers. In particular, it feels a bit weird to implement a crypto.Signer for a cipher, if the canonical package doesn't do it itself - unless there is a good reason to that I'm unaware of. There might also be some opportunity to simplify x/crypto/openpgp a bit, given that all supported ciphers implement crypto.Signers at that point.

[FiloSottile]

Any reason to support DSA at all in a new feature? Actually, can we drop support for DSA from x/crypto/openpgp instead? If it weren't for the Go 1 Compatibility Promise I would be suggesting dropping crypto/dsa itself. (Related: #27883)

crypto/ and x/crypto share a philosophy of minimalism in that they only include a secure, safe and limited useful subset of the protocols they implement. Sadly, x/crypto/openpgp doesn't really match that at the moment, and I'd be much happier maintaining it if we started trimming it down. (Related: #25388)

[Merovius]

My personaly reason is mainly "I want to say that I support OpenPGP keys and feel more comfortable doing that, if I'm as close to what they support as reasonable". As for normative reasons, if we care about compliance with the spec we MUST have DSA :)

[FiloSottile]

Respecting a spec MUST is a concern, but in this case I'm going to call for removing an obsolete primitive and reducing complexity instead. A lot of the value of the Go crypto libraries is in the careful subset they implement, there are other libraries to aim for 100% compatibility.

Opened #27905 to track removal.

[Merovius]

I'm disappointed by this decision. I don't think DSA really adds significant complexity TBH - it works mostly identical to ECDSA and should be able to share almost all of the code. IMO a primitive being deprecated totally justifies not producing it any more - but consumption should still be possible. What's worse, by removing DSA from upstream, you are making it pretty much impossible to provide something even close to complete without forking the whole package, more than doubling the actual complexity.

Anyway, obviously it's not my decision. But the fact that I was willing to invest time to make everyone's
code better, only to see that effort causing this is discouraging.

[FiloSottile]

I disagree about complexity, as this issue proves it would have required changes all the way into the standard library to keep carrying support for DSA. As I mentioned on the issue, if enough users would find themselves needing a fork I am open to backing out the change. That's why I asked if you had a direct need for it yourself, or were carrying it just for completeness. (I would continue this discussion on #27905 as that's where feedback will collect.)

I am really sorry you feel like your effort was wasted, and I wouldn't want you to be discouraged from contributing further. Saying no is definitely the hardest part of my job, but I believe it's important to stay true to the philosophy of these libraries. I believe reducing complexity is an outcome that does make everyone's code better :)

By the way, gpg-agent support is something I strongly look forward to, so thank you for working on that.

[josephlr]

Given that #27905 was rejected, I think we should reopen this. Not having basic crypto.Signer support for a PrivateKey type is silly. It also makes it very difficult for packages to have support for DSA while presenting a nice external API.

For example, the https://github.com/mozilla-services/pkcs7 package would ideally just take a crypto.Signer as the signing key. However, we have to take a crypto.PrivateKey right now as the library (and the PKCS7 spec) support DSA. This hurdle was discussed here: mozilla-services/pkcs7#18 (comment)

Going forward there seem like two paths:

• Remove dsa support from Go
  • Personally, this is my prefered approach, but it was rejected in x/crypto/openpgp: remove DSA support #27905 (and would break compatiblity requirements)
• Add crypto.Signer support to dsa.PrivateKey
  • As @Merovius noted, this is a very easy change to the standard library (~5 lines)

[josephlr]

CC: @jvehent @mozmark (who work on the PKCS7 library in question)

[FiloSottile]

DSA is insecure. The overwhelming majority of the implementations only support 1024 bits keys, which like 1024 bit RSA keys are not secure anymore. No one should be using DSA. I will open a proposal to deprecate crypto/dsa.

It also makes it very difficult for packages to have support for DSA while presenting a nice external API.

That sounds great. In the Go standard library we can't remove things, but we can make them harder to use by not supporting them with nicer and newer APIs.

You mention removing DSA support would be your preferred approach. We can't do that, but we can pretend it was removed for the purposes of further development, and other applications like PKCS7 can do the same.

(For example they could drop support for this insecure key type, or failing that require wrapping the dsa.PrivateKey in a InsecureDSAKey type that implements crypto.Signer. That would let them have the nice API take a crypto.Signer, still support DSA for backwards compatibility, and require a loud opt-in for insecure key types. This feels strictly preferable to letting insecure keys flow through APIs unnoticed.)

I am firmly in support of declining this proposal, but happy for it to go through the proper process now that it's more formalized. Thanks for reopening it.

[josephlr]

DSA is insecure. The overwhelming majority of the implementations only support 1024 bits keys, which like 1024 bit RSA keys are not secure anymore. No one should be using DSA. I will open a proposal to deprecate crypto/dsa.

100% agree, and I'd endorse DSA deprcation.

I think the status of this proposal should be linked to DSA's support in Go. If we deprecate it, we close this proposal. If we don't deprecate it, we accept this proposal.

(For example they could drop support for this insecure key type, or failing that require wrapping the dsa.PrivateKey in a InsecureDSAKey type that implements crypto.Signer. That would let them have the nice API take a crypto.Signer, still support DSA for backwards compatibility, and require a loud opt-in for insecure key types. This feels strictly preferable to letting insecure keys flow through APIs unnoticed.)

This seems like the best way forward for these sorts of operations in general (not just the PKCS7 library).

[rsc]

Based on the discussion above, this sounds like a likely decline (don't add more to crypto/dsa).

[rsc]

No change in consensus, so declined.