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 Rfc3279 signature format for DSA and EcDSA #31548

Closed
krwq opened this issue Nov 22, 2019 · 7 comments · Fixed by #1612
Closed

Support Rfc3279 signature format for DSA and EcDSA #31548

krwq opened this issue Nov 22, 2019 · 7 comments · Fixed by #1612
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@krwq
Copy link
Member

krwq commented Nov 22, 2019

When DSA/EcDSA API was defined the specs did not clarify how signature should look like - current version of our API returns a concatenation of R and S (also known as IEEEP1363). Multiple specs call out to use ASN.1 encoded signature as described in Rfc3279 (DER sequence of R and S). The same format is also returned internally by OpenSSL (we internally convert it to IEEEP1363). Proposed APIs are meant to bridge the gap between two worlds and make it much easier to use those APIs in various contexts.

namespace System.Security.Cryptography {

public enum DSASignatureFormat
{
    IeeeP1363FixedFieldConcatenation,
    Rfc3279DerSequence
}

public abstract partial class DSA : System.Security.Cryptography.AsymmetricAlgorithm
{
    public byte[] SignData(byte[] data, int offset, int count, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public byte[] SignData(byte[] data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] SignDataCore(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool TrySignDataCore(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }

    public bool VerifyData(byte[] data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(byte[] data, int offset, int count, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(Stream data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifyDataCore(Stream data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }

    public byte[] CreateSignature(byte[] rgbHash, DSASignatureFormat signatureFormat) { throw null; }
    public bool TryCreateSignature(ReadOnlySpan<byte> hash, Span<byte> destination, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] CreateSignatureCore(ReadOnlySpan<byte> hash, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool TryCreateSignatureCore(ReadOnlySpan<byte> hash, Span<byte> destination, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }

    public bool VerifySignature(byte[] rgbHash, byte[] rgbSignature, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifySignature(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifySignatureCore(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat) { throw null; }

    public int GetMaxSignatureSize(DSASignatureFormat signatureFormat) { throw null; }
}

public abstract partial class ECDsa : System.Security.Cryptography.AsymmetricAlgorithm
{
    public byte[] SignData(byte[] data, int offset, int count, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public byte[] SignData(byte[] data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] SignDataCore(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool TrySignDataCore(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }

    public bool VerifyData(byte[] data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(byte[] data, int offset, int count, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(Stream data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifyDataCore(Stream data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }

    public byte[] SignHash(byte[] hash, DSASignatureFormat signatureFormat) { throw null; }
    public bool TrySignHash(ReadOnlySpan<byte> hash, Span<byte> destination, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual byte[] SignHashCore(ReadOnlySpan<byte> hash, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool TrySignHashCore(ReadOnlySpan<byte> hash, Span<byte> destination, out int bytesWritten, DSASignatureFormat signatureFormat) { throw null; }

    public bool VerifyHash(byte[] hash, byte[] signature, DSASignatureFormat signatureFormat) { throw null; }
    public bool VerifyHash(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat) { throw null; }
    protected virtual bool VerifyHashCore(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat) { throw null; }

    public int GetMaxSignatureSize(DSASignatureFormat signatureFormat) { throw null; }
}
}

cc: @bartonjs

@krwq krwq self-assigned this Nov 22, 2019
@krwq
Copy link
Member Author

krwq commented Nov 22, 2019

Full contract after these changes https://gist.github.com/krwq/b5a14288769db15335ffa079eb3b34bd to make it easier to review

@bartonjs
Copy link
Member

I'd leave the SignatureFormatter and SignatureDeformatter out of it. Those types are supposed to represent "a signature format" already, so if we wanted to do it we'd make new types.

But we haven't maintained that type hierarchy (note there isn't one for ECDsa), so we should just leave it as-is.

protected virtual byte[] SignDataCore(byte[] data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; } is redundant to the one that takes an offset and a count. I'd personally make it

 protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }

Which could be implemented in terms of TrySignDataCore if we wanted to reduce virtuals, but letting the derived type properly size the byte array is OK if you think it's important. Likewise the Stream one should be implemented in the base class and may or may not be useful to have a virtual implementation.

For VerifyData only the ReadOnlySpan one (and maybe the Stream one) should be virtual.

Similarly for SIgnHash/VerifyHash.

@krwq
Copy link
Member Author

krwq commented Nov 22, 2019

@bartonjs agreed on *Formatter. I'm ok with unifying all byte[] *Core overloads with ROS (presumably you meant to leave non-virtual as they are). The only thing is that current implementation actually makes byte[] overloads one allocation faster but presumably that doesn't matter that much considering all the crypto/hashing happening there and possibly we will optimize that in the future.

The TrySignData is kinda weird though considering user can't easily tell what the destination size should be and try & increase on error would be more expensive than getting the byte array. Perhaps we should have some utility telling the user how much they should have? (SignatureSize(SignatureFormat)?)

@bartonjs
Copy link
Member

Perhaps we should have some utility telling the user how much they should have? (SignatureSize(SignatureFormat)?)

We could add a GetMaxSignatureSize(SignatureFormat) on both DSA and ECDsa, yeah. That seems perfectly reasonable and potentially valueable. (For IEEEP1363 "Max" and "actual" are the same, for X509 it's not)

The only thing is that current implementation actually makes byte[] overloads one allocation faster but presumably that doesn't matter that much.

I'm not sure I understand. If you mean to avoid the overalloc/copy-return then two virtuals can still be defined for SignData (and similarly for SignHash/ComputeSignature):

protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }
protected virtual bool TrySignDataCore(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat) { throw null; }

@krwq
Copy link
Member Author

krwq commented Nov 25, 2019

@bartonjs I've updated the proposal with your comments. Also the diff can be seen here:
https://gist.github.com/krwq/b5a14288769db15335ffa079eb3b34bd/revisions#diff-3b67675289ab122c2f20ce3b3e912d0c

@terrajobst
Copy link
Member

terrajobst commented Nov 26, 2019

Video

  • The APIs cannot be resumed, thus no OperationStatus or bytesConsumed outputs
  • The naming is slightly different between DSA and ECDsa but this mirrors existing conventions
  • out should go last
#nullable enable
namespace System.Security.Cryptography
{
    public enum DSASignatureFormat
    {
        IeeeP1363FixedFieldConcatenation,
        Rfc3279DerSequence
    }

    public abstract partial class DSA : AsymmetricAlgorithm
    {
        public byte[] SignData(byte[] data, int offset, int count, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public byte[] SignData(byte[] data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat, out int bytesWritten);
        protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual byte[] SignDataCore(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool TrySignDataCore(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat, out int bytesWritten);

        public bool VerifyData(byte[] data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(byte[] data, int offset, int count, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(Stream data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool VerifyDataCore(Stream data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);

        public byte[] CreateSignature(byte[] rgbHash, DSASignatureFormat signatureFormat);
        public bool TryCreateSignature(ReadOnlySpan<byte> hash, Span<byte> destination, DSASignatureFormat signatureFormat, out int bytesWritten);
        protected virtual byte[] CreateSignatureCore(ReadOnlySpan<byte> hash, DSASignatureFormat signatureFormat);
        protected virtual bool TryCreateSignatureCore(ReadOnlySpan<byte> hash, Span<byte> destination, DSASignatureFormat signatureFormat, out int bytesWritten);

        public bool VerifySignature(byte[] rgbHash, byte[] rgbSignature, DSASignatureFormat signatureFormat);
        public bool VerifySignature(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat);
        protected virtual bool VerifySignatureCore(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat);

        public int GetMaxSignatureSize(DSASignatureFormat signatureFormat);
    }

    public abstract partial class ECDsa : AsymmetricAlgorithm
    {
        public byte[] SignData(byte[] data, int offset, int count, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public byte[] SignData(byte[] data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public byte[] SignData(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool TrySignData(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat, out int bytesWritten);
        protected virtual byte[] SignDataCore(ReadOnlySpan<byte> data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual byte[] SignDataCore(Stream data, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool TrySignDataCore(ReadOnlySpan<byte> data, Span<byte> destination, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat, out int bytesWritten);

        public bool VerifyData(byte[] data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(byte[] data, int offset, int count, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(Stream data, byte[] signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        public bool VerifyData(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool VerifyDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);
        protected virtual bool VerifyDataCore(Stream data, ReadOnlySpan<byte> signature, HashAlgorithmName hashAlgorithm, DSASignatureFormat signatureFormat);

        public byte[] SignHash(byte[] hash, DSASignatureFormat signatureFormat);
        public bool TrySignHash(ReadOnlySpan<byte> hash, Span<byte> destination, DSASignatureFormat signatureFormat, out int bytesWritten);
        protected virtual byte[] SignHashCore(ReadOnlySpan<byte> hash, DSASignatureFormat signatureFormat);
        protected virtual bool TrySignHashCore(ReadOnlySpan<byte> hash, Span<byte> destination, DSASignatureFormat signatureFormat, out int bytesWritten);

        public bool VerifyHash(byte[] hash, byte[] signature, DSASignatureFormat signatureFormat);
        public bool VerifyHash(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat);
        protected virtual bool VerifyHashCore(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, DSASignatureFormat signatureFormat);

        public int GetMaxSignatureSize(DSASignatureFormat signatureFormat);
    }
}

@ahsonkhan
Copy link
Member

We should consider making the out parameter (bytesWritten) as the last argument.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants