Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v2]
On Tue, 19 Oct 2021 20:32:21 GMT, Sean Mullan wrote:
>> This fix improves the exception message to better indicate when the key (and
>> not the signature algorithm) is restricted. This change also includes a few
>> other improvements:
>>
>> - The constraints checking in `AlgorithmChecker.check()` has been improved.
>> If the `AlgorithmConstraints` are an instance of
>> `DisabledAlgorithmConstraints`, the internal `permits` methods are always
>> called; otherwise the public `permits` methods are called. This makes the
>> code easier to understand, and fixes at least one case where duplicate
>> checks were being done.
>>
>> - The above change caused some of the exception messages to be slightly
>> different, so some tests that checked the error messages had to be updated
>> to reflect that.
>>
>> - AlgorithmDecomposer now stores the canonical algorithm names in a Map,
>> which fixed a bug where "RSASSA-PSS" was not being restricted properly.
>
> Sean Mullan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> - Changed names of AlgorithmDecomposer.canonicalName and
> decomposeOneHashName
> methods.
> - Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES
> Map instead of hardcoding algorithm names.
> - Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so
> that
> constraints on the key algorithm and size are checked in the check() method
> if
> the constraints are an instanceof DisabledAlgorithmConstraints.
src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 106:
> 104: // "SHA-256" and "SHA256" to make the right constraint checking.
> 105:
> 106: for (Map.Entry e :
> DECOMPOSED_DIGEST_NAMES.entrySet()) {
If you're going to change this code, you can save me a PR if you surround this
by "if (algorithm.contains("SHA") { ... }"
Its a perf change to eliminate the unnecessary map lookups when SHA isn't in
the algorithm string
src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 158:
> 156: Set elements = decomposeImpl(algorithm);
> 157:
> 158: for (Map.Entry e :
> DECOMPOSED_DIGEST_NAMES.entrySet()) {
Same "if (algorithm.contains("SHA") { ... }" comment as above
src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 159:
> 157:
> 158: for (Map.Entry e :
> DECOMPOSED_DIGEST_NAMES.entrySet()) {
> 159: hasLoop(elements, e.getKey(), e.getValue());
I think it's worth merging the contents of hasLoop() into this for loop. This
is the only method calling hasLoop() and the extra method calls are not useful.
Your addition DECOMPOSED_DIGEST_NAMES makes a merger a more reasonable
solution now than before.
-
PR: https://git.openjdk.java.net/jdk/pull/5928
RFR: 8225181: KeyStore should have a getAttributes method
Add `KeyStore::getAttributes` so that one can get the attributes of an entry without retrieving the entry first. This is especially useful for a private key entry which can only be retrieved with a password. - Commit messages: - 8225181: KeyStore should have a getAttributes method Changes: https://git.openjdk.java.net/jdk/pull/6026/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6026=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8225181 Stats: 152 lines in 6 files changed: 150 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6026/head:pull/6026 PR: https://git.openjdk.java.net/jdk/pull/6026
Integrated: 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi
On Sat, 9 Oct 2021 19:41:51 GMT, Joe Darcy wrote: > Analogous to other recent cleanups like JDK-8274393, suppress warnings for > serialization-related issues in the windows mscapi code. This pull request has now been integrated. Changeset: 926966be Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/926966be7ad91d2b4a750583c78721b2cdb26981 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/5879
Integrated: 8264849: Add KW and KWP support to PKCS11 provider
On Fri, 17 Sep 2021 23:22:21 GMT, Valerie Peng wrote: > Anyone has time to review this RFE for adding AES cipher with KW, KWP modes > support to SunPKCS11 provider? > > The main changes are in only one new class, i.e. P11KeyWrapCipher.java, which > is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When testing > against NSS library, it seems that they only support the single part enc/dec > PKCS11 APIs, so have to use a new class as existing P11Cipher class relies on > the multi part enc/dec PKCS11 APIs and do not support key wrapping/unwrapping. > > The rest are minor code refactoring and updates for the PKCS11 Exception > class. > The new regression tests are adapted from existing key wrap regression tests > for SunJCE provider. > > Thanks, > Valerie This pull request has now been integrated. Changeset: e63c1486 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/e63c1486dc00ee64dea1a76b5a44e34f06eb144f Stats: 2135 lines in 17 files changed: 2036 ins; 46 del; 53 mod 8264849: Add KW and KWP support to PKCS11 provider Reviewed-by: ascarpino - PR: https://git.openjdk.java.net/jdk/pull/5569
Integrated: 8275252: Migrate cacerts from JKS to password-less PKCS12
On Thu, 14 Oct 2021 13:36:19 GMT, Weijun Wang wrote: > The cacerts file is now a password-less PKCS12 file. This make sure old code > that uses a JKS KeyStore object can continuously load it using a null > password (in fact, any password) and see all certificates inside. This pull request has now been integrated. Changeset: bd2b41dd Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/bd2b41dd7062c50f3aaebec2137d5fdd9546c120 Stats: 82 lines in 3 files changed: 2 ins; 69 del; 11 mod 8275252: Migrate cacerts from JKS to password-less PKCS12 Reviewed-by: erikj, ihse, mullan - PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v2]
> This fix improves the exception message to better indicate when the key (and > not the signature algorithm) is restricted. This change also includes a few > other improvements: > > - The constraints checking in `AlgorithmChecker.check()` has been improved. > If the `AlgorithmConstraints` are an instance of > `DisabledAlgorithmConstraints`, the internal `permits` methods are always > called; otherwise the public `permits` methods are called. This makes the > code easier to understand, and fixes at least one case where duplicate checks > were being done. > > - The above change caused some of the exception messages to be slightly > different, so some tests that checked the error messages had to be updated to > reflect that. > > - AlgorithmDecomposer now stores the canonical algorithm names in a Map, > which fixed a bug where "RSASSA-PSS" was not being restricted properly. Sean Mullan has updated the pull request incrementally with one additional commit since the last revision: - Changed names of AlgorithmDecomposer.canonicalName and decomposeOneHashName methods. - Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES Map instead of hardcoding algorithm names. - Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so that constraints on the key algorithm and size are checked in the check() method if the constraints are an instanceof DisabledAlgorithmConstraints. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5928/files - new: https://git.openjdk.java.net/jdk/pull/5928/files/27045940..cf5a4d7f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5928=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5928=00-01 Stats: 73 lines in 3 files changed: 12 ins; 32 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/5928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5928/head:pull/5928 PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12 [v2]
> The cacerts file is now a password-less PKCS12 file. This make sure old code > that uses a JKS KeyStore object can continuously load it using a null > password (in fact, any password) and see all certificates inside. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: use a standard name - Changes: - all: https://git.openjdk.java.net/jdk/pull/5948/files - new: https://git.openjdk.java.net/jdk/pull/5948/files/0ee2c53c..285ed1b3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5948=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5948=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5948.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5948/head:pull/5948 PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12 [v2]
On Tue, 19 Oct 2021 18:49:11 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> use a standard name
>
> make/jdk/src/classes/build/tools/generatecacerts/GenerateCacerts.java line 54:
>
>> 52: public static void store(String dir, OutputStream stream) throws
>> Exception {
>> 53:
>> 54: CertificateFactory cf = CertificateFactory.getInstance("X509");
>
> Nit: better to use the standard name here: "X.509".
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12 [v2]
On Tue, 19 Oct 2021 19:48:23 GMT, Weijun Wang wrote:
>> The cacerts file is now a password-less PKCS12 file. This make sure old code
>> that uses a JKS KeyStore object can continuously load it using a null
>> password (in fact, any password) and see all certificates inside.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use a standard name
Marked as reviewed by mullan (Reviewer).
make/jdk/src/classes/build/tools/generatecacerts/GenerateCacerts.java line 54:
> 52: public static void store(String dir, OutputStream stream) throws
> Exception {
> 53:
> 54: CertificateFactory cf = CertificateFactory.getInstance("X509");
Nit: better to use the standard name here: "X.509".
-
PR: https://git.openjdk.java.net/jdk/pull/5948
Re: RFR: 8275003: Suppress warnings on non-serializable non-transient instance fields in windows mscapi
On Sat, 9 Oct 2021 19:41:51 GMT, Joe Darcy wrote: > Analogous to other recent cleanups like JDK-8274393, suppress warnings for > serialization-related issues in the windows mscapi code. Looks fine. Thanks! - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5879
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
On Tue, 19 Oct 2021 17:15:48 GMT, Anthony Scarpino
wrote:
>> Right, it's really just about using consistent message digest names so that
>> it can match for example, "SHA-1" and also "SHA1withRSA". I'll change the
>> name to something else.
>
> Was the reason for this change that hashName("RSASSA-PSS") was returning an
> RSASSAPSS?
Yes.
-
PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
On Tue, 19 Oct 2021 15:48:57 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
>> 196:
>>
>>> 194: static String canonicalName(String algorithm) {
>>> 195: return CANONICAL_NAME.getOrDefault(algorithm, algorithm);
>>> 196: }
>>
>> I'm not sure if `canonicalName` is good. Normally, we say "SHA-1" is the
>> standard name but this method changes it to "SHA1".
>
> Right, it's really just about using consistent message digest names so that
> it can match for example, "SHA-1" and also "SHA1withRSA". I'll change the
> name to something else.
Was the reason for this change that hashName("RSASSA-PSS") was returning an
RSASSAPSS?
-
PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v4]
On Tue, 19 Oct 2021 13:28:26 GMT, Aleksei Efimov wrote: >> This change implements a new service provider interface for host name and >> address resolution, so that java.net.InetAddress API can make use of >> resolvers other than the platform's built-in resolver. >> >> The following API classes are added to `java.net.spi` package to facilitate >> this: >> - `InetAddressResolverProvider` - abstract class defining a service, and >> is, essentially, a factory for `InetAddressResolver` resolvers. >> - `InetAddressResolverProvider.Configuration ` - an interface describing the >> platform's built-in configuration for resolution operations that could be >> used to bootstrap a resolver construction, or to implement partial >> delegation of lookup operations. >> - `InetAddressResolver` - an interface that defines methods for the >> fundamental forward and reverse lookup operations. >> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the >> characteristics of one forward lookup operation. >> >> More details in [JEP-418](https://openjdk.java.net/jeps/418). >> >> Testing: new and existing `tier1:tier3` tests > > Aleksei Efimov has updated the pull request incrementally with four > additional commits since the last revision: > > - Remove no longer used import from IPSupport > - Rename IPSupport.hasAddress and update it to use SocketChannel.open > - Address review comments: javadoc + code cleanup > - Address resolver bootstraping issue Changes look good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v4]
On Tue, 19 Oct 2021 13:10:15 GMT, Weijun Wang wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated comment in test > > Approved the PR. Thanks. I have a small comment on the CSR. @wangweij Updated the CSR with your comment. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v5]
> It'd be useful to have a -version option for keytool and jarsigner. Many > other JDK tools already have a -version option. This is to add -version > option to keytool and jarsigner like jar command does. > > CSR review: > https://bugs.openjdk.java.net/browse/JDK-8275174 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: fullusage test change - Changes: - all: https://git.openjdk.java.net/jdk/pull/5954/files - new: https://git.openjdk.java.net/jdk/pull/5954/files/e801c6a2..e6b360e7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5954=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5954=03-04 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5954.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5954/head:pull/5954 PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v4]
On Tue, 19 Oct 2021 13:10:15 GMT, Weijun Wang wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated comment in test > > Approved the PR. Thanks. I have a small comment on the CSR. @wangweij Thanks for reviewing the CSR and code changes. - PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs
On Thu, 5 Aug 2021 20:10:44 GMT, Weijun Wang wrote: > New `Subject` APIs `current()` and `callAs()` are created to be replacements > of `getSubject()` and `doAs()` since the latter two methods are now > deprecated for removal. > > In this implementation, by default, `current()` returns the same value as > `getSubject(AccessController.getCurrent())` and `callAs()` is implemented > based on `doAs()`. This behavior is subject to change in the future once > `SecurityManager` is removed. > > User can experiment a possible future mechanism by setting the system > property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` > method stores the subject into a `ThreadLocal` object and the `current()` > method returns it (Note: this mechanism does not work with principal-based > permissions). > > Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and > user can start switching to `callAs()` in their applications. Users can also > switch to `current()` but please note that if you used to call > `getSubject(acc)` in a `doPrivileged` call you might need to try calling > `current()` in a `doPrivilegedWithCombiner` call to see if the > `AccessControlContext` inside the call inherits the subject from the outer > one. test/jdk/javax/security/auth/Subject/FromACC.java line 9: > 7: * published by the Free Software Foundation. Oracle designates this > 8: * particular file as subject to the "Classpath" exception as provided > 9: * by Oracle in the LICENSE file that accompanied this code. Oops, no Classpath exception for this test. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs
On Wed, 18 Aug 2021 15:01:12 GMT, Sean Mullan wrote:
>> New `Subject` APIs `current()` and `callAs()` are created to be replacements
>> of `getSubject()` and `doAs()` since the latter two methods are now
>> deprecated for removal.
>>
>> In this implementation, by default, `current()` returns the same value as
>> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented
>> based on `doAs()`. This behavior is subject to change in the future once
>> `SecurityManager` is removed.
>>
>> User can experiment a possible future mechanism by setting the system
>> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()`
>> method stores the subject into a `ThreadLocal` object and the `current()`
>> method returns it (Note: this mechanism does not work with principal-based
>> permissions).
>>
>> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and
>> user can start switching to `callAs()` in their applications. Users can also
>> switch to `current()` but please note that if you used to call
>> `getSubject(acc)` in a `doPrivileged` call you might need to try calling
>> `current()` in a `doPrivilegedWithCombiner` call to see if the
>> `AccessControlContext` inside the call inherits the subject from the outer
>> one.
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 353:
>
>> 351:
>> 352: /**
>> 353: * Return the current installed subject.
>
> Nit. I know other accessor methods in Subject use the word "Return", but I
> think "Returns" is more grammatically correct and reads better when
> describing what this method does. Also just say "Returns the current subject."
I'll use "returns".
> src/java.base/share/classes/javax/security/auth/Subject.java line 355:
>
>> 353: * Return the current installed subject.
>> 354: *
>> 355: * The current installed subject is installed by the {@link #call}
>
> s/The current installed subject/The current subject/
>
> I am not sure "installed" is the best term to use, as I equate that most
> commonly with installing software on your disk. Maybe "set" or "registered"?
I thought about "associated" but cannot precisely describe what it it is
associated to. The "install" word actually comes from "install a
SecurityManager with the `System::setSecurityManager` method".
I'll use "current object".
> src/java.base/share/classes/javax/security/auth/Subject.java line 357:
>
>> 355: * The current installed subject is installed by the {@link #call}
>> 356: * or {@link #run} method. When either {@code call(subject,
>> action)} or
>> 357: * {@code run(subject, action)} is called, {@code action} is
>> executed
>
> Should this be `callAs` or `doAs`?
Correct, it should be `callAs` and `getAs`.
> src/java.base/share/classes/javax/security/auth/Subject.java line 359:
>
>> 357: * {@code run(subject, action)} is called, {@code action} is
>> executed
>> 358: * with {@code subject} as its current installed subject which can
>> be
>> 359: * retrieved by this method. After {@code action} is finished, the
>> current
>
> Just say "current subject" instead of "current installed subject" throughout.
> Once you explain how the current subject is installed (or registered), there
> is no need to repeat that term.
I see what you mean, it's only "current subject", but we can use a verb to
describe how it's set.
> src/java.base/share/classes/javax/security/auth/Subject.java line 361:
>
>> 359: * retrieved by this method. After {@code action} is finished, the
>> current
>> 360: * installed subject is reset to its previous value. The current
>> installed
>> 361: * subject is {@code null} before the first call of {@code call()}
>
> It is also null if there are no active `call` or `doAs` methods, right?
Yes, is this implied by "reset to its previous value"?
> src/java.base/share/classes/javax/security/auth/Subject.java line 369:
>
>> 367: *
>> 368: * @implNote
>> 369: * In this implementation, if a {@code SecurityManager} is allowed,
>
> We should probably link "allowed" to the section in the `SecurityManager` API
> that describes the behavior. Perhaps we could see if we could add a hyperlink.
Precisely, this "allowed" covers "-Djava.security.manager" set to "allow", "",
and a class name. I should probably be more precise.
> src/java.base/share/classes/javax/security/auth/Subject.java line 373:
>
>> 371: * {@code getSubject(AccessController.getContext())}. Otherwise,
>> the current
>> 372: * installed subject is stored in an inheritable {@code
>> ThreadLocal}.
>> 373: * Also, each of the various {@code doAs(subject, action)} and
>
> It's not clear if the "Also ..." sentence is only for the non-SM case. I
> think you could just use one sentence here, replace "Also, each" with "and
> each ...".
Oops, I think I made a mistake here. No matter if there's SM, `current` and
`getSubject`
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs
On Thu, 5 Aug 2021 20:10:44 GMT, Weijun Wang wrote:
> New `Subject` APIs `current()` and `callAs()` are created to be replacements
> of `getSubject()` and `doAs()` since the latter two methods are now
> deprecated for removal.
>
> In this implementation, by default, `current()` returns the same value as
> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented
> based on `doAs()`. This behavior is subject to change in the future once
> `SecurityManager` is removed.
>
> User can experiment a possible future mechanism by setting the system
> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()`
> method stores the subject into a `ThreadLocal` object and the `current()`
> method returns it (Note: this mechanism does not work with principal-based
> permissions).
>
> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and
> user can start switching to `callAs()` in their applications. Users can also
> switch to `current()` but please note that if you used to call
> `getSubject(acc)` in a `doPrivileged` call you might need to try calling
> `current()` in a `doPrivilegedWithCombiner` call to see if the
> `AccessControlContext` inside the call inherits the subject from the outer
> one.
Max, I am thinking now that we don't need to make the new `Subject.callAs`
method call `Subject.doAs` if an SM is not disallowed, and instead just use the
TL solution. However, we still want `Subject.current` to support both the SM
and TL case as that would be typically called by library code. My reasoning is
that applications that are calling `doAs` and depending on the SM will need to
find another solution anyway, or stay on the older releases. We probably don't
want them to transition to the new `callAs` method (which is not deprecated)
and then suddenly it stops working when the SM is removed.
src/java.base/share/classes/javax/security/auth/Subject.java line 334:
> 332:
> 333: private static enum SubjectStorage {
> 334: ACC, THREAD_LOCAL, SCOPE_LOCAL
I would leave out SCOPE_LOCAL for now.
src/java.base/share/classes/javax/security/auth/Subject.java line 352:
> 350: };
> 351:
> 352: /**
Some initial comments below. I think this will take a few iterations to refine
the text.
src/java.base/share/classes/javax/security/auth/Subject.java line 353:
> 351:
> 352: /**
> 353: * Return the current installed subject.
Nit. I know other accessor methods in Subject use the word "Return", but I
think "Returns" is more grammatically correct and reads better when describing
what this method does. Also just say "Returns the current subject."
src/java.base/share/classes/javax/security/auth/Subject.java line 355:
> 353: * Return the current installed subject.
> 354: *
> 355: * The current installed subject is installed by the {@link #call}
s/The current installed subject/The current subject/
I am not sure "installed" is the best term to use, as I equate that most
commonly with installing software on your disk. Maybe "set" or "registered"?
src/java.base/share/classes/javax/security/auth/Subject.java line 357:
> 355: * The current installed subject is installed by the {@link #call}
> 356: * or {@link #run} method. When either {@code call(subject, action)}
> or
> 357: * {@code run(subject, action)} is called, {@code action} is executed
Should this be `callAs` or `doAs`?
src/java.base/share/classes/javax/security/auth/Subject.java line 359:
> 357: * {@code run(subject, action)} is called, {@code action} is executed
> 358: * with {@code subject} as its current installed subject which can be
> 359: * retrieved by this method. After {@code action} is finished, the
> current
Just say "current subject" instead of "current installed subject" throughout.
Once you explain how the current subject is installed (or registered), there is
no need to repeat that term.
src/java.base/share/classes/javax/security/auth/Subject.java line 361:
> 359: * retrieved by this method. After {@code action} is finished, the
> current
> 360: * installed subject is reset to its previous value. The current
> installed
> 361: * subject is {@code null} before the first call of {@code call()}
It is also null if there are no active `call` or `doAs` methods, right?
src/java.base/share/classes/javax/security/auth/Subject.java line 369:
> 367: *
> 368: * @implNote
> 369: * In this implementation, if a {@code SecurityManager} is allowed,
We should probably link "allowed" to the section in the `SecurityManager` API
that describes the behavior. Perhaps we could see if we could add a hyperlink.
src/java.base/share/classes/javax/security/auth/Subject.java line 373:
> 371: * {@code getSubject(AccessController.getContext())}. Otherwise, the
> current
> 372: * installed subject is stored in an inheritable {@code ThreadLocal}.
> 373: * Also, each
RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs
New `Subject` APIs `current()` and `callAs()` are created to be replacements of `getSubject()` and `doAs()` since the latter two methods are now deprecated for removal. In this implementation, by default, `current()` returns the same value as `getSubject(AccessController.getCurrent())` and `callAs()` is implemented based on `doAs()`. This behavior is subject to change in the future once `SecurityManager` is removed. User can experiment a possible future mechanism by setting the system property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` method stores the subject into a `ThreadLocal` object and the `current()` method returns it (Note: this mechanism does not work with principal-based permissions). Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and user can start switching to `callAs()` in their applications. Users can also switch to `current()` but please note that if you used to call `getSubject(acc)` in a `doPrivileged` call you might need to try calling `current()` in a `doPrivilegedWithCombiner` call to see if the `AccessControlContext` inside the call inherits the subject from the outer one. - Commit messages: - deprecate doAs, use new APIs in src and test - Merge branch 'master' into 8267108 - old untouched. new same as old by default, or use TL when a sys prop is set - multiple implementations - 8267108: Alternate Subject.getSubject API that does not depend on Security Manager APIs Changes: https://git.openjdk.java.net/jdk/pull/5024/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5024=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267108 Stats: 653 lines in 20 files changed: 452 ins; 92 del; 109 mod Patch: https://git.openjdk.java.net/jdk/pull/5024.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5024/head:pull/5024 PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
On Tue, 19 Oct 2021 15:26:52 GMT, Sean Mullan wrote: >> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line >> 48: >> >>> 46:"SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224", >>> 47:"SHA512/224", "SHA-512/256", "SHA512/256"); >>> 48: >> >> Do you want to support the "SHA" -> "SHA1" mapping? > > These should be standard digest names as specified by the disabled algorithm > security property syntax. SHA is an alias. OK, I saw the default in `CANONICAL_NAME.getOrDefault(algorithm, algorithm)` and thought non-standard names are also allowed. - PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
On Tue, 19 Oct 2021 14:34:25 GMT, Weijun Wang wrote:
>> This fix improves the exception message to better indicate when the key (and
>> not the signature algorithm) is restricted. This change also includes a few
>> other improvements:
>>
>> - The constraints checking in `AlgorithmChecker.check()` has been improved.
>> If the `AlgorithmConstraints` are an instance of
>> `DisabledAlgorithmConstraints`, the internal `permits` methods are always
>> called; otherwise the public `permits` methods are called. This makes the
>> code easier to understand, and fixes at least one case where duplicate
>> checks were being done.
>>
>> - The above change caused some of the exception messages to be slightly
>> different, so some tests that checked the error messages had to be updated
>> to reflect that.
>>
>> - AlgorithmDecomposer now stores the canonical algorithm names in a Map,
>> which fixed a bug where "RSASSA-PSS" was not being restricted properly.
>
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
> 48:
>
>> 46:"SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224",
>> 47:"SHA512/224", "SHA-512/256", "SHA512/256");
>> 48:
>
> Do you want to support the "SHA" -> "SHA1" mapping?
These should be standard digest names as specified by the disabled algorithm
security property syntax. SHA is an alias.
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line
> 196:
>
>> 194: static String canonicalName(String algorithm) {
>> 195: return CANONICAL_NAME.getOrDefault(algorithm, algorithm);
>> 196: }
>
> I'm not sure if `canonicalName` is good. Normally, we say "SHA-1" is the
> standard name but this method changes it to "SHA1".
Right, it's really just about using consistent message digest names so that it
can match for example, "SHA-1" and also "SHA1withRSA". I'll change the name to
something else.
-
PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
On Wed, 13 Oct 2021 13:42:25 GMT, Sean Mullan wrote:
> This fix improves the exception message to better indicate when the key (and
> not the signature algorithm) is restricted. This change also includes a few
> other improvements:
>
> - The constraints checking in `AlgorithmChecker.check()` has been improved.
> If the `AlgorithmConstraints` are an instance of
> `DisabledAlgorithmConstraints`, the internal `permits` methods are always
> called; otherwise the public `permits` methods are called. This makes the
> code easier to understand, and fixes at least one case where duplicate checks
> were being done.
>
> - The above change caused some of the exception messages to be slightly
> different, so some tests that checked the error messages had to be updated to
> reflect that.
>
> - AlgorithmDecomposer now stores the canonical algorithm names in a Map,
> which fixed a bug where "RSASSA-PSS" was not being restricted properly.
src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 48:
> 46:"SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224",
> 47:"SHA512/224", "SHA-512/256", "SHA512/256");
> 48:
Do you want to support the "SHA" -> "SHA1" mapping?
src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 196:
> 194: static String canonicalName(String algorithm) {
> 195: return CANONICAL_NAME.getOrDefault(algorithm, algorithm);
> 196: }
I'm not sure if `canonicalName` is good. Normally, we say "SHA-1" is the
standard name but this method changes it to "SHA1".
-
PR: https://git.openjdk.java.net/jdk/pull/5928
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Sun, 17 Oct 2021 21:39:06 GMT, Mark Sheppard wrote: > I think that a hostname is constant while a host is up, but it can be > changed, and when changed a host restart is required. I don't think it is > quite as dynamic as has been suggested, but I open to correction. It is possible to change a host name without host restart. What has been tested: - Checked O/S type - Linux - `hostnamectl` cmd was used to change a host name - In output below there is only one `jshell` instance running. jshell> InetAddress.getLocalHost() $1 ==> test-host-name/192.168.0.155 $ hostnamectl set-hostname 'test-host-name-changed' # Need to sleep for 5 seconds since local host # is cached for 5 seconds since last resolution $ sleep 5 jshell> InetAddress.getLocalHost() $2 ==> test-host-name-changed/192.168.0.155 I believe it proves that we need to treat a host name as dynamically changing information. - PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 17:19:26 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> test/lib/jdk/test/lib/net/IPSupport.java line 88:
>
>> 86: } catch (SocketException se2) {
>> 87: return false;
>> 88: }
>
> The implementation of this method now conflicts with its name. Maybe we
> should pass a `Class` as parameter, and construct the
> loopback address from there. IIRC the issue with the original implementation
> was that it would say that IPv6 was not supported if IPv6 had only been
> disabled on the loopback interface - and that caused trouble because the
> native implementation of the built-in resolver had a different view of the
> situation - and saw that an IPv6 stack was available on the machine. Maybe
> this would deserve a comment too - so that future maintainers can figure out
> what we are trying to do here.
Thanks for a good suggestion: renamed to `isSupported`, changed parameter type
to `Class addressType` and updated it to use
`SocketChannel.open(ProtocolFamily pf)`
[API](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/channels/SocketChannel.html#open(java.net.ProtocolFamily))
added in `15` to check if the plafrom supports addresses of specified address
type. Pushed as 95a21e57fbe8be147d23e6a6901ae307e8237cbb.
All new tests (especially `LookupPolicyMappingTest`) pass in environment with
`IPv6` addresses disabled.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 17:09:46 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> test/jdk/java/net/spi/InetAddressResolverProvider/providers/recursive/recursive.init.provider/impl/InetAddressUsageInGetProviderImpl.java
> line 38:
>
>> 36: String localHostName;
>> 37: try {
>> 38: localHostName = InetAddress.getLocalHost().getHostName();
>
> Try to call InetAddress.getLocalHost().getHostName(); twice here.
New `BootstrapResolverUsageTest` test is added to trigger the bootstrap
resolver problem. `InetAddress.getByName` calls with unique names are used
instead of `InetAddress.getLocalHost()` to avoid caching of LHN resolution
results. Added in cd19ecd63838d3c2b713ed66d41107fe01a2d3e6.
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v4]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with four additional commits since the last revision: - Remove no longer used import from IPSupport - Rename IPSupport.hasAddress and update it to use SocketChannel.open - Address review comments: javadoc + code cleanup - Address resolver bootstraping issue - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/d302a49a..ac0d2184 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=02-03 Stats: 245 lines in 5 files changed: 180 ins; 35 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
On Fri, 15 Oct 2021 14:25:02 GMT, Daniel Fuchs wrote:
>> Aleksei Efimov has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Add @since tags to new API classes
>> - Add checks and test for empty stream resolver results
>
> src/java.base/share/classes/java/net/InetAddress.java line 231:
>
>> 229: * The first provider found will be used to instantiate the {@link
>> InetAddressResolver InetAddressResolver}
>> 230: * by invoking the {@link
>> InetAddressResolverProvider#get(InetAddressResolverProvider.Configuration)}
>> 231: * method. The instantiated {@code InetAddressResolver} will be
>> installed as the system-wide
>
> Maybe "... method. The **returned** {@code InetAddressResolver} will be
> installed ..."
Changed as suggested in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.
> src/java.base/share/classes/java/net/InetAddress.java line 486:
>
>> 484: return cns;
>> 485: } finally {
>> 486: bootstrapResolver = null;
>
> This is incorrect. This will set bootstrapResolver to null in the case where
> you return it at line 468 - which means that a second call will then find it
> null. I believe you want to set it to null in the finally clause only if you
> reached line 470. You could achieve this by declaring a local variable - e.g
> `InetAddressResolver tempResolver = null;` before entering the try-finally
> then set that variable at line 470 `return tempResolver = bootstrapResolver;`
> and in finally do `if (tempResolver == null) bootsrapResolver = null;`
>
> To test this you could try to call e.g. `InetAddress.getByName` twice in
> succession in your test: I believe it will fail with the code as it stands,
> but will succeed with my proposed changes...
Good catch, thank you Daniel. Added a test for that and fixed it in
cd19ecd63838d3c2b713ed66d41107fe01a2d3e6. Now the `bootstrapResolver` field is
set back to `null` in finally block only if a call to `InetAddress.resolver()`
entered a resolver initialization part.
> src/java.base/share/classes/java/net/InetAddress.java line 860:
>
>> 858: return host;
>> 859: }
>> 860: } catch (RuntimeException | UnknownHostException e) {
>
> This may deserve a comment: resolver.lookUpHostName and
> InetAddress.getAllbyName0 delegate to the system-wide resolver, which could
> be custom, and we treat any unexpected RuntimeException thrown by the
> provider at that point as we would treat an UnknownHostException or an
> unmatched host name.
Agree - comment added as part of 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.
> src/java.base/share/classes/java/net/InetAddress.java line 1063:
>
>> 1061: public Stream lookupAddresses(String host,
>> LookupPolicy policy)
>> 1062: throws UnknownHostException {
>> 1063: Objects.requireNonNull(host);
>
> Should we also double check that `policy` is not null? Or maybe assert it?
Yes, we should since custom resolvers might call the built-in one with `null` -
added non-null check in 648e65b .
> src/java.base/share/classes/java/net/InetAddress.java line 1203:
>
>> 1201: + " as hosts file " + hostsFile + " not found
>> ");
>> 1202: }
>> 1203: // Check number of found addresses:
>
> I wonder if the logic here could be simplified by first checking whether only
> IPv4 addresses are requested, and then if only IPv6 addresses are requested.
>
> That is - evacuate the simple cases first and then only deal with the more
> complex case where you have to combine the two lists...
Tried to simplify it in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe
-
PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v4]
On Tue, 19 Oct 2021 06:26:17 GMT, Hai-May Chao wrote: >> It'd be useful to have a -version option for keytool and jarsigner. Many >> other JDK tools already have a -version option. This is to add -version >> option to keytool and jarsigner like jar command does. >> >> CSR review: >> https://bugs.openjdk.java.net/browse/JDK-8275174 > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Updated comment in test Approved the PR. Thanks. I have a small comment on the CSR. - PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v4]
On Tue, 19 Oct 2021 06:26:17 GMT, Hai-May Chao wrote: >> It'd be useful to have a -version option for keytool and jarsigner. Many >> other JDK tools already have a -version option. This is to add -version >> option to keytool and jarsigner like jar command does. >> >> CSR review: >> https://bugs.openjdk.java.net/browse/JDK-8275174 > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Updated comment in test Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v4]
> It'd be useful to have a -version option for keytool and jarsigner. Many > other JDK tools already have a -version option. This is to add -version > option to keytool and jarsigner like jar command does. > > CSR review: > https://bugs.openjdk.java.net/browse/JDK-8275174 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: Updated comment in test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5954/files - new: https://git.openjdk.java.net/jdk/pull/5954/files/c0a71608..e801c6a2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5954=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5954=02-03 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5954.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5954/head:pull/5954 PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v3]
> It'd be useful to have a -version option for keytool and jarsigner. Many > other JDK tools already have a -version option. This is to add -version > option to keytool and jarsigner like jar command does. > > CSR review: > https://bugs.openjdk.java.net/browse/JDK-8275174 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: jarsigner only prints version and ignores other options - Changes: - all: https://git.openjdk.java.net/jdk/pull/5954/files - new: https://git.openjdk.java.net/jdk/pull/5954/files/4593b7a8..c0a71608 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5954=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5954=01-02 Stats: 13 lines in 2 files changed: 4 ins; 5 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5954.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5954/head:pull/5954 PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8272163: Add -version option to keytool and jarsigner [v2]
On Tue, 19 Oct 2021 03:40:55 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix -version in jarsigner and update tests
>
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 314:
>
>> 312: if (version) {
>> 313: doPrintVersion();
>> 314: }
>
> So if both `-version` and `file.jar` are provided, jarsigner will both sign
> the jar and print the version. This is probably OK but I prefer the `java`
> style that only prints the version.
Ok, change made to have jarsigner to only print the version and ignore other
options.
-
PR: https://git.openjdk.java.net/jdk/pull/5954
