Re: RFR 8026953: Add support for MS Cryptography next generation (CNG) (step 1)
On 10/25/2018 11:09 PM, Weijun Wang wrote: Hi Mike Thanks for the feedback. I understand the current SunMSCAPI implementation recognizes RSA keys only and it's certainly incorrect to put something like getModulus() and getPublicExponent() in a general CKey class. They will be fixed later. When I have more sub class names, I'll definitely use them. You can see I've moved some CSignature methods into CSignature$RSA. I just haven't done it everywhere. OK. We'll still need a base CKey for a CNG key, no matter what the underlying algorithm is. Since CNG uses the same NCRYPT_KEY_HANDLE type for different types of keys, we will do something similar on the Java side. Since the current CPublicKey and CPrivateKey are very light, it will be easy to move the content into algorithm-specific classes. This is where I think you need to fix the structure: abstract class CKey public class CRSAPublicKey extends CKey implements RSAKey, RSAPublicKey, PublicKey public class CRSAExtractablePrivateKey extends CKey implements RSAKey, RSAPrivateKey, PrivateKey[,RSAMultiPrimePrivateCrt | RSAPrivateCrtKey] public class CRSAPrivateKey extends CKey implements RSAKey, PrivateKey public class CECPublicKey extends CKey implements ECKey, ECPublicKey, PublicKey public class CECExtractablePrivateKey extends CKey implements ECKey, PrivateKey public class CECPrivateKey extends CKey implements ECKey, ECPrivateKey, ECPrivateKey Note the two different versions of the private keys to match up with the key handling bits as well as some additional interfaces that may be needed to be added to support the underlying provider's requirements for the RSA keys. Also, I'm looking ahead a little bit and thinking about how the JCA would use the windows PCP (Platform Crypto Provider) which uses the TPM to enforce hardware security. It would be useful if you didn't have to re-write everything just because of a different underlying Windows provider. (There's a PCP development kit that's got some useful sample code that might help a little bit with refactoring the JCA MSCAPI provider even for the existing code). E.g. eventually supporting an MSCAPI-PCP provider shouldn't require all new code. The main reason I want to take this first step is that after some study on CNG I make some progress and also see some blockers. For example, I am able to expose a EC CNG key stored in Windows-MY now and use it to sign/verify. However, I still don't know how to import external keys inside there (certmgr.exe can so it's possible). Until now, the most requested function is to use existing keys in signatures and I want to start working on it. The first thing I noticed then is that the current class names are unsuitable and I think a refactoring will make them look better. AFAICT, you're not going to be able to use any external key without importing it or running it through a key factory. The main class you're going to be using is NCryptImportKey (or alternately BCryptImportKeyPair). Once I start working on the next step, I'll need to have different sub classes in CKey and CSignature. I even have an alternative plan to ditch CPublicKey and use the PublicKey classes in SunEC and SunRsaSign. This was actually already used in the RSASSA-PSS signature handling in SunMSCAPI we added into JDK 11 in the last minute. So you just use software classes in another provider for encrypting/verifying? To be honest this sounds messy and may come back to bite you down the road. As for KeyFactory, we do not have an urgent requirement to use external keys in a CNG Signature object or store them into Windows-MY. Also, we can use the one in SunRsaSign. Hmm... one of the more common things is to move around .p12 files with your certs and keys. They can be imported by the Windows tools - it would be *really* nice if you can do the same thing with the Java provider. Thanks again. --Max On Oct 26, 2018, at 1:25 AM, Michael StJohns wrote: Hi Max - For the same reason I was pushing back on Adam's EC provider I think I need to push back here. You're recasting an RSAPublicKey to just a PublicKey and making it difficult to move key material in and out of the MSCAPI proivider. Same thing with the private key. For example, in the CPublicKey class you still have "getModulus()" and "getPublicExponent()", but to get at them you'd have to use CPublicKey rather than PublicKey. And looking forward, I'm not sure how you actually implement the EC classes here using this model. I'd suggest not making the change this way and overloading the existing classes, but instead adding the appropriate provider classes for new key types as you implement support for them. E.g. Keep CRSAKey, CRSAPublicKey and CRSAPrivateKey as distinct classes, add CECKey, CECPublicKey and CECPrivateKey when you get there. Are you missing a KeyFactory class as well? Lastly, you may want to change the subclass/methods for CSignature (and proba
Re: Code Review Request, JDK-8212738, Incorrectly named signature scheme ecdsa_secp512r1_sha512
Looks good to me Tony On 10/29/2018 10:41 AM, Xuelei Fan wrote: Hi, Please review the update: http://cr.openjdk.java.net/~xuelei/8212738/webrev.00/ The signature algorithm name should be ""ecdsa_secp521r1_sha512", instead of "ecdsa_secp512r1_sha512". No new regression test. Trivial update, no impact on the behaviors except the debug log message. Thanks, Xuelei
Code Review Request, JDK-8212738, Incorrectly named signature scheme ecdsa_secp512r1_sha512
Hi, Please review the update: http://cr.openjdk.java.net/~xuelei/8212738/webrev.00/ The signature algorithm name should be ""ecdsa_secp521r1_sha512", instead of "ecdsa_secp512r1_sha512". No new regression test. Trivial update, no impact on the behaviors except the debug log message. Thanks, Xuelei
Re: Hashing in Java and Java Cryptography Architecture (JCA) design
There are several ways in which JCA is deficient when viewed from the
standpoint of modern API design. At some point, we will probably want to
develop a new API that doesn't have these issues. I expect this to
require significant effort, though, and this sort of work gets lower
priority compared to implementing new algorithms/standards and TLS 1.3
work. So it is hard to tell if/when anyone will have time to work on this.
FWIW, I partially agree with the complaint about provider selection
complexity. I think the provider/algorithm selection mechanism is
important, but perhaps not everyone needs it, and it shouldn't clutter
up the core APIs. A more typical pattern for this sort of thing is to
include a configuration layer on top of the core
abstractions/implementation, and all of the provider selection
complexity would go in this layer.
I also agree that there are ways to improve the core APIs, including
making them more functional/streaming, using fluent syntax, etc.
On 10/27/2018 6:23 AM, John Newman wrote:
Not to be rude to Thomas but has anyone else have any thoughts on this?
--Janez
*From:* security-dev on behalf
of Thomas Lußnig
*Sent:* Tuesday, October 23, 2018 8:24:33 PM
*To:* security-dev@openjdk.java.net
*Subject:* Re: Hashing in Java and Java Cryptography Architecture
(JCA) design
Hi,
even if it looks complicated for you the idea is that your code is not
hard wired to MD5 or SHA1 but in ideal case
it is configured. Than you do not know in advance if the selected digest
is available.
On the other hand no one say that you can create your own helper/tools
class.
The idea is that you are not fixed to one algo but what if you say "MD4"
or "POLY1305" only some algorithm
where available at coding time thy can be removed later (maybe even via
policy) or newly been added. For
this there is the NoSuchAlgorithmException to show the developer there
can be some error.
RuntimeException would be ignored and may crash the whole program or task.
Gruß Thomas
class DIGEST {
static byte[] SHA1(byte[]... args) {
...
}
}
then you can simply call DIGEST.SHA1(a,b,c)
On 23.10.2018 19:50:44, John Newman wrote:
> This seems to me overly complicated for a simple task of
> instantiating a MessageDigest object:
>
> MessageDigest md = null;
> try {
> md = MessageDigest.getInstance("SHA-1");
> } catch (NoSuchAlgorithmException nsae) {}
>
> Couldn't MessageDigest simply be an *interface* and
> the SHA funcionality a special *implementation*, like so:
>
> MessageDigest md = new ShaMessageDigest();
>
> ?
>
> But instead we have these factory methods (accross all of the JCA
> core classes) which throw exceptions, polluting the code. Is the
> Provider abstraction really needed? The only real benefit I see is
> neater class names.
>
> In fact - couldn't we get rid of the entire Java Cryptography
Architecture (JCA)
> as it is and redesign it to be more object oriented? For example,
couldn't this:
>
> byte [][] args = //...
> MessageDigest md = //..
> md.update(args[0])
> md.update(args[1]);
> md.digest();
>
> become this:
>
> byte [][] args = //...
> MessageDigest md = //..
> md.update(args[0]).update(args[1]).digest();
>
> or maybe even this:
>
> MessageDigest md = //..
> byte [][] args = //...
> new IntermediateDigest(
> md,
> args[0],
> args[1]
> ).bytes()
>
> where IntermediateDigest itself could be an argument to
MessageDigest's update()
> method, making things like H(H(x), y) look much more compact and
readable?
>
>
> --Janez
RE: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)
Hi Alan, security guys,
I've proposed a CSR for this change now:
https://bugs.openjdk.java.net/browse/JDK-8213082.
I also updated the webrev, simplifying jdk.nio.zipfs.ZipUtils.permsFromFlags
and eliminating the WeakHashMap:
http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/
Since I've decoupled the changes to java.util.zip and jartool from those in
jdk.zipfs, we're discussing only jdk.zipfs here.
The implication of my change is that when working with files backed by the nio
FileSystemProvider (java.nio.file.spi.FileSystemProvider) named "jar", which is
the alias for zipfs, the files will support attributes of type
java.nio.file.attribute.PosixFilePermissions ("posix:permissions").
It basically means that some methods of java.nio.file.Files that would return
null or UnsupportedOperationException before would find an implementation now.
Examples:
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getPosixFilePermissions(java.nio.file.Path,java.nio.file.LinkOption...)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#setPosixFilePermissions(java.nio.file.Path,java.util.Set)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#readAttributes(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)
* With class
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributes.html
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getFileAttributeView(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)
* With class
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributeView.html
Thanks in advance for reviewing.
Best regards
Christoph
From: Alan Bateman
Sent: Montag, 29. Oktober 2018 13:06
To: Langer, Christoph ; core-libs-dev
; security-dev@openjdk.java.net; Xueming Shen
Cc: Volker Simonis ; Andrew Luo
; nio-dev
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions (was:
Enhance jdk.nio.zipfs to support Posix File Permissions)
On 29/10/2018 09:26, Langer, Christoph wrote:
:
As per request from Alan, I'm adding security-dev to get a review from security
perspective.
For security-dev then I think it would be better to write-up a summary of the
overall proposal and the implications for applications/libraries that use the
APIs and the jar tool. The security discussion points all relate to capture and
propagation of file permissions.
-Alan
Re: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)
On 29/10/2018 09:26, Langer, Christoph wrote: : As per request from Alan, I’m adding security-dev to get a review from security perspective. For security-dev then I think it would be better to write-up a summary of the overall proposal and the implications for applications/libraries that use the APIs and the jar tool. The security discussion points all relate to capture and propagation of file permissions. -Alan
RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)
Hi, here's an update of my webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.1/ I added synchronization to the updating of permCache in ZipUtils.java to avoid concurrent modifications. As per request from Alan, I'm adding security-dev to get a review from security perspective. Thanks Christoph From: Langer, Christoph Sent: Freitag, 26. Oktober 2018 17:13 To: core-libs-dev ; nio-dev ; 'Xueming Shen' Cc: Schmelter, Ralf ; 'Volker Simonis' Subject: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions Hi, please review this enhancement of jdk.nio.zipfs to support Posix file permissions. Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.0/ Bug: https://bugs.openjdk.java.net/browse/JDK-8213031 I had already posted this change as part of a larger fix for "6194856: Zip Files lose ALL ownership and permissions of the files" [1]. With the original proposal I was also addressing the java.util.zip API and the jar tool. However, I've decided to split this endeavor into 2 parts. While I still want to follow up on java.util.zip and jar, I'd like to bring the enhancement to jdk.zipfs in first. Both places don't have dependencies to each other and since zipfs does not change an external API, I guess the bar here is lower. Maybe it is even a candidate for down-porting to jdk11u in the future. After my fix, zipfs will support the PosixFileAttributeView. Posix file attributes can't be fully supported since owner and group are not handled in zip files. So methods supposed to get these attributes will return an UnsupportedOperationException. But the posix permissions will now be correctly handled, that is stored into and read from the zip file. @Sherman: Following suggestions from your review, I removed the test with the binary zip file. I initially thought it is a good idea to test on a well-known file. However, testWriteAndReadArchiveWithPosixPerms tests both, writing a zip file and reading it again so I guess test coverage is quite complete here. Furthermore, I made some public declarations in ZipUtils package private which should suffice. I also tried to address your performance concerns with permsToFlags and permsFromFlags. In permsToFlags I will now simply iterate to the set of permissions and add the flags to the return value. It's probably more performant than the streaming approach and doesn't look much worse in the code. In permsFromFlags I added a cache of permission sets which should avoid constant calls to the streaming. However, as the return value needs to be mutable, I have to clone the cached permissions. Maybe it would have the same or even better performance if we iteratively fill a new set of permissions each time permsToFlags gets called. What do you think? Do we need a CSR for this patch? Thanks & Best regards Christoph [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/055971.html
