AsnWriter should provide an API to write non-normalized integers

[filipnavara]

Currently AsnWriter offers couple of methods for writing integers represented using ReadOnlySpan<byte>:

public void WriteInteger(ReadOnlySpan<byte> value);
public void WriteIntegerUnsigned(ReadOnlySpan<byte> value);
public void WriteInteger(Asn1Tag tag, ReadOnlySpan<byte> value);
public void WriteIntegerUnsigned(Asn1Tag tag, ReadOnlySpan<byte> value);

These methods throw CryptographicException when the value is not properly padded, ie. the integer starts with a zero, followed by a byte < 0x80.

Nearly all of the usages of these methods use extra code to strip this padding. Examples include:

https://github.com/dotnet/corefx/blob/1e2dfe2c2ef9ca04a46e38f6645021e5da39ffe3/src/System.Security.Cryptography.Algorithms/src/Internal/Cryptography/Helpers.cs#L109-L144

https://github.com/dotnet/corefx/blob/85c4b4bdae8095c3681d4cd91e00894f26c3baf6/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/CertificateRequest.cs#L610-L630

https://github.com/dotnet/corefx/blob/5812dcbef2cde02a8213139085deefe666376f2c/src/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Rfc3161TimestampRequest.cs#L259-L283

I found no usages of WriteInteger[Unsigned] that rely on the padding check except for the unit tests that check that specific behavior.

I propose that these WriteInteger[Unsigned] overloads should strip the padding instead of throwing an exception on it. Alternatively, if there's a compelling reason to keep the current API with the checks, offer a different API that does the normalization.

Furthermore, the WriteIntegerUnsigned overloads above are not exposed from AsnSerializer. If the normalization is enabled in some way it should be available for the serializer as well.

Of the three normalization code snippets above two of them had bugs, which were spotted only after careful review (one of them landed in master, the other was spotted during pull request review). This suggests that it's a bit of code that's prone to be written incorrectly and a good API should ideally prevent the users from writing incorrect code.

/cc @bartonjs

[bartonjs]

The design of these methods is that they are used to transparently write down data that was successfully read from an equivalent call to the GetIntegerBytes method on AsnReader. The "wait, this is illegal" check is to help prevent streams being crossed. (It also would hopefully fail often enough under an endianness problem that the caller bug could quickly be assessed)

The thinking here is that most consumers of the API would have their integers in a semantic form (a numeric primitive or BigInteger), and use those overloads; and that once you have the integer in a bytes you're "supposed to know what you're doing". WriteIntegerUnsigned got added due to a "ugh, why do I need to copy this to a temporary buffer one byte larger when I just need the writer to write down a leading zero?".

Perhaps a reasonable thing would be to replace WriteIntegerUnsigned with something like WriteIntegerNormalized([Asn1Tag tag, ]ReadOnlyMemory\<byte\> bigEndianBytes, bool isUnsigned) (maybe with a default value for isUnsigned, maybe with another bool for endianness)

Seems like it would be awkward to express in the serializer or generator, though.

[filipnavara]

I am not sure whether most of the consumers actually have the numbers in the normalized form. Use cases that I found were the following:

• Generating a randomized number in fixed size buffer and writing it down. The random part can contain zeroes in the beginning or start with byte >= 0x80.
• Writing cryptographic key parameters that are stored in zero-padded buffers.

The WriteIntegerNormalized API would solve these use cases. On the other hand if I already have a value in normalized form then WriteIntegerNormalized would work for it too. Hence I am not sure if there's a value in API that checks the value and throws an exception for non-normalized forms.

[filipnavara]

Assuming your API design with WriteInteger and WriteIntegerNormalized and no isBigEndian parameter, exposing the API in the generator could be done with normalize attribute with no (default) / yes / unsigned values for <asn:Integer> element.

Assuming an API that always normalizes and exposes WriteInteger and WriteIntegerUnsigned a simple unsigned attribute with true / false (default) values would work.

[bartonjs]

I mean the expected user of AsnWriter who is writing integers, and that I expect them to use WriteInteger(long), WriteInteger(ulong), or WriteInteger(BigInteger) more than WriteInteger(ReadOnlyMemory\<byte>).

The difference between WriteInteger(bytes) and WriteIntegerNormalized(bytes) is one is "I promise this is well-formed, (please throw me if it isn't)" and the other is "I need to tell you more data to help you coax the bytes into the right form".

The addition of the word "Normalized" is a signal to say "data coercion is going to happen here", since coercion is itself a source of bugs (not that the coercion is necessarily wrong, but was perhaps the wrong one for the specific context).

Mainly, I'm trying to avoid magic.

[filipnavara]

I am fine with avoiding magic and showing the intent. In fact I like your proposal for WriteIntegerNormalized after giving it more thought. I just wonder whether there's an actual use case for WriteInteger(ReadOnlyMemory<byte>) once WriteIntegerNormalized is present.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq Notify danmosemsft if you want to be subscribed.