RFR: 8249783: Simplify DerValue and DerInputStream [v2]

[Valerie Peng]

On Sat, 26 Sep 2020 00:04:28 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/util/DerValue.java line 284:
>> 
>>> 282:
>>> 283:     /**
>>> 284:      * Parse an ASN.1/BER encoded datum. The entire encoding must hold exactly
>> 
>> Are you intentionally put BER here because allowBER=true?
>
> Yes. In fact I'm not quite confident on our usage of allowBER. In a lot of cases we are actually quite strict. Since
> this code change is meant to be a refactoring and does not intend to fix many things, I don't intent to make many
> behavior change.

We have to be strict in "sensitive" area such as signatures. The parsing code seems to be still mainly DER. It was
never fully BER, but just some. It's good to keep behavior change minimum as this is like a re-write and may already
have some unintentional changes.

>> src/java.base/share/classes/sun/security/util/DerValue.java line 322:
>> 
>>> 320:         } else if ((lenByte & 0x080) == 0x00) { // short form, 1 byte datum
>>> 321:             length = lenByte;
>>> 322:         } else {                     // long form or indefinite
>> 
>> This should just the long form. Isn't the indefinite case covered by the first if-check?
>
> Correct.

Fix the comment then?

>> src/java.base/share/classes/sun/security/util/DerValue.java line 226:
>> 
>>> 224:         this.end = end;
>>> 225:         this.allowBER = allowBER;
>>> 226:         this.data = new DerInputStream(this);
>> 
>> this.data used to just contain the "value" part of the DerValue, now it's the whole DerValue (including tag and length
>> in addition to value). Is its usage still compatible?
>
> The constructor `DerInputStream(DerValue)` has not read tag at all. I can rewrite it the last line to `new
> DerInputStream(buffer, start, end - start, allowBER)`.

Yes, I think it is better to just use the buffer/start/end-start/allowBER for this.data.

-------------

PR: https://git.openjdk.java.net/jdk/pull/232