Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v3]
On Tue, 17 Nov 2020 15:37:18 GMT, Weijun Wang wrote: >> Without this method, `PSSParameterSpec::toString` shows something like: >> MD: SHA-256 >> MGF: java.security.spec.MGF1ParameterSpec@77b52d12 >> SaltLength: 32 >> TrailerField: 1 >> This is ugly. >> >> Noreg-trivial. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > choose a record-style format Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v3]
On Tue, 17 Nov 2020 19:36:31 GMT, Sean Mullan wrote: >> New commit and the output will look like: >> PSSParameterSpec[hashAlgorithm=SHA-256, >> maskGenAlgorithm=MGF1ParameterSpec[hashAlgorithm=SHA-256], saltLength=32, >> trailerField=1] >> I'm using the ASN.1 field names here. (Precisely the name in MGF1 is >> OAEP-PSSDigestAlgorithms but that's too long). > >> New commit and the output will look like: >> >> ``` >> PSSParameterSpec[hashAlgorithm=SHA-256, >> maskGenAlgorithm=MGF1ParameterSpec[hashAlgorithm=SHA-256], saltLength=32, >> trailerField=1] >> ``` >> >> I'm using the ASN.1 field names here. (Precisely the name in MGF1 is >> OAEP-PSSDigestAlgorithms but that's too long). > > I like it. Looks good. @seanjmullan Can you approve it then? - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v3]
On Tue, 17 Nov 2020 15:34:37 GMT, Weijun Wang wrote: > New commit and the output will look like: > > ``` > PSSParameterSpec[hashAlgorithm=SHA-256, > maskGenAlgorithm=MGF1ParameterSpec[hashAlgorithm=SHA-256], saltLength=32, > trailerField=1] > ``` > > I'm using the ASN.1 field names here. (Precisely the name in MGF1 is > OAEP-PSSDigestAlgorithms but that's too long). I like it. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v3]
On Mon, 16 Nov 2020 16:30:04 GMT, Sean Mullan wrote: >> minor comment on spacing for MGF1 toString() > >> Do you want me to consolidate `PSSParameterSpec::toString` into one line? >> Multi-line toString is usually not very friendly to debug output (esp for >> grep). > > Good question. Readability is important though. I think it depends on the > number of fields and we should try to use a consistent format if possible. > Something like X509Certificate.toString would be very unfriendly if it > returned a one-line String, or if the toString impls of different fields used > different formatting. New commit and the output will look like: PSSParameterSpec[hashAlgorithm=SHA-256, maskGenAlgorithm=MGF1ParameterSpec[hashAlgorithm=SHA-256], saltLength=32, trailerField=1] I'm using the ASN.1 field names here. (Precisely the name in MGF1 is OAEP-PSSDigestAlgorithms but that's too long). - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v3]
> Without this method, `PSSParameterSpec::toString` shows something like: > MD: SHA-256 > MGF: java.security.spec.MGF1ParameterSpec@77b52d12 > SaltLength: 32 > TrailerField: 1 > This is ugly. > > Noreg-trivial. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: choose a record-style format - Changes: - all: https://git.openjdk.java.net/jdk/pull/1208/files - new: https://git.openjdk.java.net/jdk/pull/1208/files/3b0a4c83..a3ab69e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1208&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1208&range=01-02 Stats: 12 lines in 3 files changed: 1 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/1208.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1208/head:pull/1208 PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 22:04:21 GMT, Sean Mullan wrote: >> I like the 2nd one better, but I'll see how Sean thinks about the verbose >> record-style toString. > > 99% of the time, it will probably be one of the MGF1ParameterSpec constants. > For those, perhaps it would be nice to just use the constant: "MGF: > MGF1ParameterSpec.SHA1" and for anything else, "MGF: MGF1ParameterSpec > [digestAlgorithm: ...>". If this is too complicated, then I am ok with the > above. This is doable and probably will be fast if I just keep checking `if (this == MGF1ParameterSpec.SHA1) return "MGF1ParameterSpec.SHA1"` but the code will be quite long. There are 11 constants. Maybe if this class were an enumeration... - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 18:20:02 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line
>> 168:
>>
>>> 166: @Override
>>> 167: public String toString() {
>>> 168: return "MGF1:" + mdName;
>>
>> I would replace "MGF1" or perhaps add "DigestAlgorithm" which is the name of
>> the attribute. Is it necessary to print that this is an MGF1?
>> PSSParameterSpec does not print that it is an RSASSA-PSS-params, and also
>> prints "MGF", so it seems there would be some duplication. It almost seems
>> like we should have some rules regarding how these parameters are printed
>> out so everything is consistent.
>>
>> Or perhaps it makes sense to have brackets around the fields. Otherwise when
>> you chain several toStrings together, it makes it more difficult to discern
>> when one field ends and the next begins. Hmm.
>
> "MGF1" is only one kind of MGF and we might see "MGF2" in the future so it's
> worth printing out. Of course, `PSSParameterSpec` already has a
> `getMGFAlgorithm()` so the name can either be printed in the `toString` of
> the outer data type or the inner one.
>
> As for `DigestAlgorithm` I don't think it's necessary because it's the only
> field for MGF1 so there's no ambiguity.
>
> That said, we can try to provide some consistency here. If the types had been
> defined as records with
> static record MGF1Params(String messageDigest) {}
> static record PssParams(String messageDigest, String mgfAlgorithm,
> MGF1Params mgfParams, int saltLength, int trailerField) {}
> The automatically generated `toString` would output something like
> PssParams[messageDigest=SHA-1, mgfAlgorithm=MGF1,
> mgfParams=MGF1Params[messageDigest=SHA-1], saltLength=20, trailerField=1]
> It's a little verbose. Do you like this style?
I like the brackets surrounding the fields of each type. I think we should try
to use brackets in the toString methods of security classes as there is often a
deep hierarchy of objects and the brackets helps to see that. I also like
seeing the actual type for non-primitives, and not an abbreviated form. As for
the names of the fields, I'm ok with whatever seems most reasonable, the ASN.1
names, or the method or parameter names.
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 20:49:15 GMT, Weijun Wang wrote: >> How about just MGF: MGF1SHA-256 or MGF: MGF1 SHA-256? >> First combines into a single string, the second uses space instead of ":". >> Just a suggestion. > > I like the 2nd one better, but I'll see how Sean thinks about the verbose > record-style toString. 99% of the time, it will probably be one of the MGF1ParameterSpec constants. For those, perhaps it would be nice to just use the constant: "MGF: MGF1ParameterSpec.SHA1" and for anything else, "MGF: MGF1ParameterSpec [digestAlgorithm: ...>". If this is too complicated, then I am ok with the above. - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 18:37:13 GMT, Valerie Peng wrote: >> The current MGF part shows `MGF: MGF1:SHA-256`. An extra space would looks a >> little strange, and you don't know if the key is `MGF: MGF1` or `MGF`. I'd >> rather keep the current output. > > How about just MGF: MGF1SHA-256 or MGF: MGF1 SHA-256? > First combines into a single string, the second uses space instead of ":". > Just a suggestion. I like the 2nd one better, but I'll see how Sean thinks about the verbose record-style toString. - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 18:07:39 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line
>> 168:
>>
>>> 166: @Override
>>> 167: public String toString() {
>>> 168: return "MGF1:" + mdName;
>>
>> do you want to insert a space after ':' ?
>
> The current MGF part shows `MGF: MGF1:SHA-256`. An extra space would looks a
> little strange, and you don't know if the key is `MGF: MGF1` or ``MGF`. I'd
> rather keep the current output.
How about just MGF: MGF1SHA-256 or MGF: MGF1 SHA-256?
First combines into a single string, the second uses space instead of ":". Just
a suggestion.
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 16:19:16 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> make PSSParameterSpec one line
>>
>> only in patch2:
>> unchanged:
>
> src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line
> 168:
>
>> 166: @Override
>> 167: public String toString() {
>> 168: return "MGF1:" + mdName;
>
> I would replace "MGF1" or perhaps add "DigestAlgorithm" which is the name of
> the attribute. Is it necessary to print that this is an MGF1?
> PSSParameterSpec does not print that it is an RSASSA-PSS-params, and also
> prints "MGF", so it seems there would be some duplication. It almost seems
> like we should have some rules regarding how these parameters are printed out
> so everything is consistent.
>
> Or perhaps it makes sense to have brackets around the fields. Otherwise when
> you chain several toStrings together, it makes it more difficult to discern
> when one field ends and the next begins. Hmm.
"MGF1" is only one kind of MGF and we might see "MGF2" in the future so it's
worth printing out. Of course, `PSSParameterSpec` already has a
`getMGFAlgorithm()` so the name can either be printed in the `toString` of the
outer data type or the inner one.
As for `DigestAlgorithm` I don't think it's necessary because it's the only
field for MGF1 so there's no ambiguity.
That said, we can try to provide some consistency here. If the types had been
defined as records with
static record MGF1Params(String messageDigest) {}
static record PssParams(String messageDigest, String mgfAlgorithm,
MGF1Params mgfParams, int saltLength, int trailerField) {}
The automatically generated `toString` would output something like
PssParams[messageDigest=SHA-1, mgfAlgorithm=MGF1,
mgfParams=MGF1Params[messageDigest=SHA-1], saltLength=20, trailerField=1]
It's a little verbose. Do you like this style?
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 08:14:00 GMT, Sean Coffey wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> make PSSParameterSpec one line
>>
>> only in patch2:
>> unchanged:
>
> src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line
> 168:
>
>> 166: @Override
>> 167: public String toString() {
>> 168: return "MGF1:" + mdName;
>
> do you want to insert a space after ':' ?
The current MGF part shows `MGF: MGF1:SHA-256`. An extra space would looks a
little strange, and you don't know if the key is `MGF: MGF1` or ``MGF`. I'd
rather keep the current output.
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 14:43:26 GMT, Sean Coffey wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> make PSSParameterSpec one line >> >> only in patch2: >> unchanged: > > minor comment on spacing for MGF1 toString() > Do you want me to consolidate `PSSParameterSpec::toString` into one line? > Multi-line toString is usually not very friendly to debug output (esp for > grep). Good question. Readability is important though. I think it depends on the number of fields and we should try to use a consistent format if possible. Something like X509Certificate.toString would be very unfriendly if it returned a one-line String, or if the toString impls of different fields used different formatting. - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 14:25:20 GMT, Weijun Wang wrote:
>> Without this method, `PSSParameterSpec::toString` shows something like:
>> MD: SHA-256
>> MGF: java.security.spec.MGF1ParameterSpec@77b52d12
>> SaltLength: 32
>> TrailerField: 1
>> This is ugly.
>>
>> Noreg-trivial.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> make PSSParameterSpec one line
>
> only in patch2:
> unchanged:
src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line 168:
> 166: @Override
> 167: public String toString() {
> 168: return "MGF1:" + mdName;
I would replace "MGF1" or perhaps add "DigestAlgorithm" which is the name of
the attribute. Is it necessary to print that this is an MGF1? PSSParameterSpec
does not print that it is an RSASSA-PSS-params, and also prints "MGF", so it
seems there would be some duplication. It almost seems like we should have some
rules regarding how these parameters are printed out so everything is
consistent.
Or perhaps it makes sense to have brackets around the fields. Otherwise when
you chain several toStrings together, it makes it more difficult to discern
when one field ends and the next begins. Hmm.
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 14:25:20 GMT, Weijun Wang wrote:
>> Without this method, `PSSParameterSpec::toString` shows something like:
>> MD: SHA-256
>> MGF: java.security.spec.MGF1ParameterSpec@77b52d12
>> SaltLength: 32
>> TrailerField: 1
>> This is ugly.
>>
>> Noreg-trivial.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> make PSSParameterSpec one line
>
> only in patch2:
> unchanged:
src/java.base/share/classes/java/security/spec/PSSParameterSpec.java line 224:
> 222: public String toString() {
> 223: StringBuilder sb = new StringBuilder();
> 224: sb.append("MD: " + mdName + ", ")
I would replace "MD" with "DigestAlgorithm" to be consistent with the names of
the other fields.
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
On Mon, 16 Nov 2020 14:25:20 GMT, Weijun Wang wrote:
>> Without this method, `PSSParameterSpec::toString` shows something like:
>> MD: SHA-256
>> MGF: java.security.spec.MGF1ParameterSpec@77b52d12
>> SaltLength: 32
>> TrailerField: 1
>> This is ugly.
>>
>> Noreg-trivial.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> make PSSParameterSpec one line
>
> only in patch2:
> unchanged:
minor comment on spacing for MGF1 toString()
src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line 168:
> 166: @Override
> 167: public String toString() {
> 168: return "MGF1:" + mdName;
do you want to insert a space after ':' ?
-
PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec [v2]
> Without this method, `PSSParameterSpec::toString` shows something like: > MD: SHA-256 > MGF: java.security.spec.MGF1ParameterSpec@77b52d12 > SaltLength: 32 > TrailerField: 1 > This is ugly. > > Noreg-trivial. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: make PSSParameterSpec one line only in patch2: unchanged: - Changes: - all: https://git.openjdk.java.net/jdk/pull/1208/files - new: https://git.openjdk.java.net/jdk/pull/1208/files/a78ee87e..3b0a4c83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1208&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1208&range=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1208.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1208/head:pull/1208 PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec
On Fri, 13 Nov 2020 21:18:30 GMT, Weijun Wang wrote: >> Without this method, `PSSParameterSpec::toString` shows something like: >> MD: SHA-256 >> MGF: java.security.spec.MGF1ParameterSpec@77b52d12 >> SaltLength: 32 >> TrailerField: 1 >> This is ugly. >> >> Noreg-trivial. > > Do you want me to consolidate `PSSParameterSpec::toString` into one line? > Multi-line toString is usually not very friendly to debug output (esp for > grep). tidying PSSParameterSpec::toString into 1 line would be useful. thanks! - PR: https://git.openjdk.java.net/jdk/pull/1208
Re: RFR: 8256363: Define toString() for MGF1ParameterSpec
On Fri, 13 Nov 2020 21:13:54 GMT, Weijun Wang wrote: > Without this method, `PSSParameterSpec::toString` shows something like: > MD: SHA-256 > MGF: java.security.spec.MGF1ParameterSpec@77b52d12 > SaltLength: 32 > TrailerField: 1 > This is ugly. > > Noreg-trivial. Do you want me to consolidate `PSSParameterSpec::toString` into one line? Multi-line toString is usually not very friendly to debug output (esp for grep). - PR: https://git.openjdk.java.net/jdk/pull/1208
RFR: 8256363: Define toString() for MGF1ParameterSpec
Without this method, `PSSParameterSpec::toString` shows something like: MD: SHA-256 MGF: java.security.spec.MGF1ParameterSpec@77b52d12 SaltLength: 32 TrailerField: 1 This is ugly. Noreg-trivial. - Commit messages: - 8256363: Define toString() for MGF1ParameterSpec Changes: https://git.openjdk.java.net/jdk/pull/1208/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1208&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256363 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1208.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1208/head:pull/1208 PR: https://git.openjdk.java.net/jdk/pull/1208
