Re: RFR: 8249783: Simplify DerValue and DerInputStream [v2]
On Sat, 19 Sep 2020 00:36:23 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Enhance DerValue::getOctetString to be able to read multi-level
>> constructed value.
>
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 62:
>
>> 60: final byte[] data;
>> 61: final int start;
>> 62: final int end;
>
> Well, end is really "start + length", the data range is from start to (end
> -1) (inclusive).
Will add some comments.
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 69:
>
>> 67: int mark;
>> 68:
>> 69: public DerInputStream(byte[] data, int start, int length, boolean
>> allowBER) {
>
> Add javadoc for the new impl? Do we need to validate things like data !=
> null, start and length have valid values? Or
> assuming that since all its callers are internal, no need to check.
I don't intend to add any check because callers are internal. I can add a
comment.
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 100:
>
>> 98: * @return the read DerValue.
>> 99: * @throws IOException if a DerValue cannot be constructed starting
>> from this position
>> 100: * because of byte shortage or encoding error.
>
> Add javadoc for the new impl?
OK.
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 90:
>
>> 88: }
>> 89:
>> 90: public byte[] toByteArray() {
>
> Add javadoc? Returns the remaining unread bytes? The name may lead people to
> expect the bytes returned are the one
> passing to the constructor.
Well. This method is sometimes used to read remaining bytes and sometimes used
to read all (suppose none has been read
yet). I will clarify.
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 103:
>
>> 101: */
>> 102: public DerValue getDerValue() throws IOException {
>> 103: DerValue result = new DerValue(
>
> Check (this.end - this.pos > 0) before calling DerValue()?
new DerValue() would fail in this case.
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 139:
>
>> 137: // does not accept constructed OCTET STRING.
>> 138: DerValue v = getDerValue();
>> 139: if (v.tag != DerValue.tag_OctetString) {
>
> Only this method checks tag of the DerValue?
Yes, this is because `DerValue::getOctetString` allows constructed value but
`DerInputStream::getOctetString` only
accepts primitive value. This is for compatibility.
All other methods will have tag checked inside the corresponding DerValue
method. Do you prefer a fast fail?
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 207:
>
>> 205: }
>> 206:
>> 207: static int getLength(InputStream in) throws IOException {
>
> This and other getLength(...) methods seems out-of-place with the new
> implementation code?
Yes, I should do some refactoring. Both methods are internal (to these 2
classes).
> src/java.base/share/classes/sun/security/util/DerInputStream.java line 280:
>
>> 278: * a later call to reset will return here.
>> 279: */
>> 280: public void mark(int value) { mark = pos; }
>
> This seems very strange, having an int argument which is not used?
`ByteArrayInoutStream::mark` also does this. When the data is all there and no
need to decide how many to remember,
this argument is useless. I'll rename it to dummy.
> src/java.base/share/classes/sun/security/util/DerValue.java line 163:
>
>> 161: // should call withTag() or data() instead (do not use the data
>> field).
>> 162:
>> 163: public /*final*/ byte tag;
>
> Conceptually, the class is far from immutable if the tag is public and
> non-final. Any caller could have changed the tag
> value. Is it that we are keeping all public fields/methods the same for max
> compatibility-sake? It'd be nice if it can
> be real immutable.
Yes, I should have said that this class can be used as an immutable class if
you do not use this and that. We will
decide if we can remove those usages and make this really immutable in the
future.
> src/java.base/share/classes/sun/security/util/DerValue.java line 170:
>
>> 168:
>> 169: // Unsafe. Legacy. Never null.
>> 170: final public DerInputStream data;
>
> nit: public final for sake of consistency? If not meant to be used anymore,
> mark it clearly across all relevant methods
> with deprecated javadoc tag?
public for compatibility, and final because it won't need to change. I'll see
how many `@deprecated` I can add. Or,
I'll see if I can move all will-be-deprecated code together.
> 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
Re: RFR: 8249783: Simplify DerValue and DerInputStream [v2]
On Fri, 25 Sep 2020 02:47:59 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/sun/security/util/DerValue.java line 638:
>>
>>> 636: }
>>> 637: if (end == start) {
>>> 638: throw new IOException("No padding");
>>
>> Well, I find the original error message is clearer: Invalid encoding: zero
>> length bit string. Just the "No padding" may
>> be somewhat unclear since no padding is needed when it's multiple of 8. Or,
>> maybe something like
>> "DerValue.getBitString, empty value".
>
> It also seems strange that it only checks that length !=0. The spec of
> BitString seems to suggest the length must
> be >=2 where the first byte is the number of padding bits. It seems that the
> right check should be (end - start) > 1?
I'll look into it. If there's anything wrong, will fix and add a regression
test. What if there is zero bit?
And yes, the old exception message is better. Don't know why I modified it.
-
PR: https://git.openjdk.java.net/jdk/pull/232
Re: RFR: 8249783: Simplify DerValue and DerInputStream [v2]
On Fri, 18 Sep 2020 21:25:28 GMT, Weijun Wang wrote: >> This code change rewrites DerValue into a mostly immutable class and >> simplifies DerInputStream as a wrapper for a >> series of DerValues objects. DerInputBuffer is removed. >> All existing methods of DerValue and DerInputStream should still work with >> the exact same behavior, except for a few >> places where bugs are fixed. For example, Indefinite length must be used >> with a constructed tag. >> Except for the ObjectIdentifier class where DerInputBuffer is directly >> referenced, no other code is touched. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Enhance DerValue::getOctetString to be able to read multi-level constructed > value. Thanks a lot for your detailed code review. I'll push a new commit early next week. - PR: https://git.openjdk.java.net/jdk/pull/232
Re: RFR: 8249783: Simplify DerValue and DerInputStream [v2]
On Fri, 18 Sep 2020 21:25:28 GMT, Weijun Wang wrote:
>> This code change rewrites DerValue into a mostly immutable class and
>> simplifies DerInputStream as a wrapper for a
>> series of DerValues objects. DerInputBuffer is removed.
>> All existing methods of DerValue and DerInputStream should still work with
>> the exact same behavior, except for a few
>> places where bugs are fixed. For example, Indefinite length must be used
>> with a constructed tag.
>> Except for the ObjectIdentifier class where DerInputBuffer is directly
>> referenced, no other code is touched.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Enhance DerValue::getOctetString to be able to read multi-level constructed
> value.
src/java.base/share/classes/sun/security/util/DerInputStream.java line 69:
> 67: int mark;
> 68:
> 69: public DerInputStream(byte[] data, int start, int length, boolean
> allowBER) {
Add javadoc for the new impl? Do we need to validate things like data != null,
start and length have valid values? Or
assuming that since all its callers are internal, no need to check.
src/java.base/share/classes/sun/security/util/DerInputStream.java line 100:
> 98: * @return the read DerValue.
> 99: * @throws IOException if a DerValue cannot be constructed starting
> from this position
> 100: * because of byte shortage or encoding error.
Add javadoc for the new impl?
src/java.base/share/classes/sun/security/util/DerInputStream.java line 90:
> 88: }
> 89:
> 90: public byte[] toByteArray() {
Add javadoc? Returns the remaining unread bytes? The name may lead people to
expect the bytes returned are the one
passing to the constructor.
src/java.base/share/classes/sun/security/util/DerInputStream.java line 62:
> 60: final byte[] data;
> 61: final int start;
> 62: final int end;
Well, end is really "start + length", the data range is from start to (end -1)
(inclusive).
src/java.base/share/classes/sun/security/util/DerInputStream.java line 103:
> 101: */
> 102: public DerValue getDerValue() throws IOException {
> 103: DerValue result = new DerValue(
Check (this.end - this.pos > 0) before calling DerValue()?
src/java.base/share/classes/sun/security/util/DerInputStream.java line 139:
> 137: // does not accept constructed OCTET STRING.
> 138: DerValue v = getDerValue();
> 139: if (v.tag != DerValue.tag_OctetString) {
Only this method checks tag of the DerValue?
src/java.base/share/classes/sun/security/util/DerInputStream.java line 280:
> 278: * a later call to reset will return here.
> 279: */
> 280: public void mark(int value) { mark = pos; }
This seems very strange, having an int argument which is not used?
src/java.base/share/classes/sun/security/util/DerInputStream.java line 207:
> 205: }
> 206:
> 207: static int getLength(InputStream in) throws IOException {
This and other getLength(...) methods seems out-of-place with the new
implementation code?
test/jdk/sun/security/util/DerValue/DeepOctets.java line 56:
> 54: Utils.runAndCheckException(
> 55: () -> new DerInputStream(input).getOctetString(),
> 56: IOException.class);
It seems that the existing impl already differs and you are just adding a
regression test to highlight the difference,
right?
src/java.base/share/classes/sun/security/util/DerValue.java line 163:
> 161: // should call withTag() or data() instead (do not use the data
> field).
> 162:
> 163: public /*final*/ byte tag;
Conceptually, the class is far from immutable if the tag is public and
non-final. Any caller could have changed the tag
value. Is it that we are keeping all public fields/methods the same for max
compatibility-sake? It'd be nice if it can
be real immutable.
src/java.base/share/classes/sun/security/util/DerValue.java line 170:
> 168:
> 169: // Unsafe. Legacy. Never null.
> 170: final public DerInputStream data;
nit: public final for sake of consistency? If not meant to be used anymore,
mark it clearly across all relevant methods
with deprecated javadoc tag?
src/java.base/share/classes/sun/security/util/DerValue.java line 294:
> 292:
> 293: /**
> 294: * Parse an ASN.1/BER encoded datum from a byte array.
DER instead of BER?
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?
src/java.base/share/classes/sun/security/util/DerValue.java line 301:
> 299: * @param allowBER whether BER is allowed
> 300: * @param allowMore whether extra bytes are allowed after the
> encoded datum.
> 301: * If true, {@code len} can be bigger than the
> length og
typo: og =>of?
Re: RFR: 8249783: Simplify DerValue and DerInputStream [v2]
On Fri, 25 Sep 2020 02:38:40 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Enhance DerValue::getOctetString to be able to read multi-level
>> constructed value.
>
> src/java.base/share/classes/sun/security/util/DerValue.java line 638:
>
>> 636: }
>> 637: if (end == start) {
>> 638: throw new IOException("No padding");
>
> Well, I find the original error message is clearer: Invalid encoding: zero
> length bit string. Just the "No padding" may
> be somewhat unclear since no padding is needed when it's multiple of 8. Or,
> maybe something like
> "DerValue.getBitString, empty value".
It also seems strange that it only checks that length !=0. The spec of
BitString seems to suggest the length must
be >=2 where the first byte is the number of padding bits. It seems that the
right check should be (end - start) > 1?
-
PR: https://git.openjdk.java.net/jdk/pull/232
Re: RFR: 8253637: Update EC removal
On Fri, 25 Sep 2020 20:38:12 GMT, Daniel D. Daugherty wrote: >> Hi, >> >> I need a quick review for two changes. The primary is a failure that shows >> up on linux-aarch64 because the systems do >> not have NSS. The default was to allow all curves tested, which before the >> native library removal was ok. The second >> was a missed changeset that got left in the local repo that helped find the >> curve name on an exception message. Thanks >> >> Tony > > @ascarpino - Thanks for removing the ProblemList entry that I added > with JDK-8253659. I did check the status of your bug (JDK-8253637) > but apparently I did that just before you started working on it today > (about 1400 ET was the last time I checked it). > > My apologies. @dcubed-ojdk That's ok. I had been working on it all morning (PT). If I realized you were looking at the bug status I would have changed it right away. Though given my PR has been sitting for a while, maybe on the list was for the best. - PR: https://git.openjdk.java.net/jdk/pull/366
Re: RFR: 8253637: Update EC removal
On Fri, 25 Sep 2020 18:56:00 GMT, Anthony Scarpino wrote: > Hi, > > I need a quick review for two changes. The primary is a failure that shows > up on linux-aarch64 because the systems do > not have NSS. The default was to allow all curves tested, which before the > native library removal was ok. The second > was a missed changeset that got left in the local repo that helped find the > curve name on an exception message. Thanks > > Tony @ascarpino - Thanks for removing the ProblemList entry that I added with JDK-8253659. I did check the status of your bug (JDK-8253637) but apparently I did that just before you started working on it today (about 1400 ET was the last time I checked it). My apologies. - PR: https://git.openjdk.java.net/jdk/pull/366
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 19:37:25 GMT, Daniel D. Daugherty wrote: >> Looks like a trivial change to me. > > @iklam - thanks for the fast review. Tony - sorry about that. The last time I checked your bug (about 2 hours ago) it wasn't "in progress". Please update your fix to remove the test from the ProblemList. - PR: https://git.openjdk.java.net/jdk/pull/362
Re: RFR: 8253637: Update EC removal [v2]
> Hi, > > I need a quick review for two changes. The primary is a failure that shows > up on linux-aarch64 because the systems do > not have NSS. The default was to allow all curves tested, which before the > native library removal was ok. The second > was a missed changeset that got left in the local repo that helped find the > curve name on an exception message. Thanks > > Tony Anthony Scarpino has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'ecfix' of github.com:ascarpino/jdk into ecfix - 8253637: Update EC removal - left behind message change - remove problem list entry - 8253637: Update EC removal - left behind message change - Changes: - all: https://git.openjdk.java.net/jdk/pull/366/files - new: https://git.openjdk.java.net/jdk/pull/366/files/eff89cf0..3c1cc898 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=366=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=366=00-01 Stats: 126 lines in 3 files changed: 124 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/366.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/366/head:pull/366 PR: https://git.openjdk.java.net/jdk/pull/366
Integrated: 8252377: Incorrect encoding for EC AlgorithmIdentifier
On Tue, 22 Sep 2020 22:21:20 GMT, Hai-May Chao wrote: > This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the > parameters field instead of encoding a > Null tag. This pull request has now been integrated. Changeset: 0e855fe5 Author:Hai-May Chao Committer: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/0e855fe5 Stats: 125 lines in 2 files changed: 124 ins; 0 del; 1 mod 8252377: Incorrect encoding for EC AlgorithmIdentifier Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/312
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
I had the fix in review if you could have waited. Tony On 9/25/20 12:30 PM, Daniel D.Daugherty wrote: Reduce noise in CI Tier2 by ProblemListing this test. - Commit messages: - Add 'aarch64' to the generic-ARCH comment so folks know what to use. - Use the correct bug ID: 8253637. - 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64 Changes: https://git.openjdk.java.net/jdk/pull/362/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=362=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253659 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/362.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/362/head:pull/362 PR: https://git.openjdk.java.net/jdk/pull/362
Integrated: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 17:15:01 GMT, Daniel D. Daugherty wrote: > Reduce noise in CI Tier2 by ProblemListing this test. This pull request has now been integrated. Changeset: 9150b902 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/9150b902 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64 Reviewed-by: iklam - PR: https://git.openjdk.java.net/jdk/pull/362
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 19:32:57 GMT, Ioi Lam wrote: >> Marked as reviewed by iklam (Reviewer). > > Looks like a trivial change to me. @iklam - thanks for the fast review. - PR: https://git.openjdk.java.net/jdk/pull/362
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 17:15:01 GMT, Daniel D. Daugherty wrote: > Reduce noise in CI Tier2 by ProblemListing this test. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/362
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 19:31:36 GMT, Ioi Lam wrote: >> Reduce noise in CI Tier2 by ProblemListing this test. > > Marked as reviewed by iklam (Reviewer). Looks like a trivial change to me. - PR: https://git.openjdk.java.net/jdk/pull/362
RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
Reduce noise in CI Tier2 by ProblemListing this test. - Commit messages: - Add 'aarch64' to the generic-ARCH comment so folks know what to use. - Use the correct bug ID: 8253637. - 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64 Changes: https://git.openjdk.java.net/jdk/pull/362/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=362=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253659 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/362.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/362/head:pull/362 PR: https://git.openjdk.java.net/jdk/pull/362
Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
On Fri, 25 Sep 2020 17:15:01 GMT, Daniel D. Daugherty wrote: > Reduce noise in CI Tier2 by ProblemListing this test. My Tier2 test job ran sun/security/ec/TestEC.java on all regular Oracle platforms except for linux-aarch64. I also updated the generic-ARCH header comment to include 'aarch64' in the list so folks don't have to wonder whether to use 'aarch64' or 'arm' or 'arm64'. - PR: https://git.openjdk.java.net/jdk/pull/362
RFR: 8253637: Update EC removal
Hi, I need a quick review for two changes. The primary is a failure that shows up on linux-aarch64 because the systems do not have NSS. The default was to allow all curves tested, which before the native library removal was ok. The second was a missed changeset that got left in the local repo that helped find the curve name on an exception message. Thanks Tony - Commit messages: - 8253637: Update EC removal - left behind message change Changes: https://git.openjdk.java.net/jdk/pull/366/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=366=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253637 Stats: 6 lines in 3 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/366.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/366/head:pull/366 PR: https://git.openjdk.java.net/jdk/pull/366
Re: RFR: 8252377: Incorrect encoding for EC AlgorithmIdentifier [v3]
On Fri, 25 Sep 2020 04:21:04 GMT, Hai-May Chao wrote: >> This change fixes the DER encoding for ECDSA AlgorithmIdentifier to omit the >> parameters field instead of encoding a >> Null tag. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Added comment for RFC Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/312
Integrated: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Mon, 21 Sep 2020 08:19:28 GMT, Alexey Bakhtin wrote: > Hi, > > Plaese review JDK-8245527 fix which implements LDAP Channel Binding support > for Java GSS/Kerberos. > Initial review is available at core-devs: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html > This version removes "tls-unique" CB type from the list of possible channel > binding types. The only supported type is > "tls-server-end-point" > CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 > > Thank you > Alexey This pull request has now been integrated. Changeset: cfa3f749 Author:Alexey Bakhtin URL: https://git.openjdk.java.net/jdk/commit/cfa3f749 Stats: 543 lines in 10 files changed: 531 ins; 0 del; 12 mod 8245527: LDAP Channel Binding support for Java GSS/Kerberos Reviewed-by: dfuchs, aefimov, mullan - PR: https://git.openjdk.java.net/jdk/pull/278
