Negative test case for constructed, definite-length octet string

[dwhjames]

Ensure octet strings with constructed, definite-length method return an error, rather than an incorrect decoding.

[dwhjames]

@tarcieri @dishmaker,

This is incomplete, but here’s an attempt at ensuring that constructed, definite-length method encoded octet strings are detected and rejected.

Seeking feedback; I made a few attempts but finally realized what you, @dishmaker, meant by your comment

• pkcs7+der: mixed BER/DER encoding with indefinite length #779 (comment)

As the Length struct was updated

• https://github.com/RustCrypto/formats/pull/1894/files#diff-4c0263de51fe68e8ec20b2284c27914950feea8eebef0352189bdb44e3a09501

I thought it might be fair game to draft a change to the Header struct

[tarcieri]

Propagating the original constructed bit through the Header sounds good to me

[tarcieri]

Hmm, not sure how I feel about the mixed visibility here. Maybe make it pub for now and perhaps we could have a followup to make all of the fields private? (I imagine that would be a bit more invasive)

[dwhjames]

Yep, definitely ugly.

Revised in 544e397

---

and perhaps we could have a followup to make all of the fields private? (I imagine that would be a bit more invasive)

I counted 24 occurrences in the der crate; for now I also added accessor methods so that new code has the option to use them in the event that the fields become private.

[tarcieri]

Maybe just go through with it, if it seems mostly internal to the der crate

[dwhjames]

I went through with it: 0c5789e

However, tests are failing due to at least
https://github.com/RustCrypto/signatures/blob/1ff759e67a1675d759e901fdb5903a07a1d7caec/ecdsa/src/der.rs#L206-L210

It looks like this change would have to be phased in?

Should I

1. roll back all the changes for using accessor methods rather than directly accessing struct fields, OR
2. roll back making the fields private but keep the pattern of using accessor methods rather than direct field access
   ?