seanjmullan commented on code in PR #271:
URL:
https://github.com/apache/santuario-xml-security-java/pull/271#discussion_r1575198060
##########
src/main/java/org/apache/xml/security/encryption/KeyDerivationMethod.java:
##########
@@ -38,9 +43,21 @@
public interface KeyDerivationMethod {
/**
- * Returns the algorithm URI of this <code>KeyDerivationMethod</code>.
+ * Returns the algorithm URI of this <code>KeyDerivationMethod</code>
*
* @return the algorithm URI of this <code>KeyDerivationMethod</code>
*/
String getAlgorithm();
+
+ /**
+ * Returns the KDF parameters used by the key derivation algorithm.
+ * Currently supported types are:
+ * {@link org.apache.xml.security.encryption.params.ConcatKDFParams} and
+ * {@link org.apache.xml.security.encryption.params.HKDFParams}
Review Comment:
There should be a period at the end of the sentence here.
##########
src/main/java/org/apache/xml/security/encryption/KeyDerivationMethod.java:
##########
@@ -38,9 +43,21 @@
public interface KeyDerivationMethod {
/**
- * Returns the algorithm URI of this <code>KeyDerivationMethod</code>.
+ * Returns the algorithm URI of this <code>KeyDerivationMethod</code>
Review Comment:
There should be a period at the end of the sentence here.
##########
src/main/java/org/apache/xml/security/encryption/keys/content/derivedKey/KeyDerivationMethodImpl.java:
##########
@@ -71,29 +73,43 @@ public String getAlgorithm() {
return getLocalAttribute(EncryptionConstants._ATT_ALGORITHM);
}
- public ConcatKDFParamsImpl getConcatKDFParams() throws
XMLSecurityException {
- if (concatKDFParams != null) {
- return concatKDFParams;
- }
+ @Override
+ public KDFParams getKDFParams() throws XMLSecurityException {
- Element concatKDFParamsElement =
- XMLUtils.selectXenc11Node(getElement().getFirstChild(),
EncryptionConstants._TAG_CONCATKDFPARAMS, 0);
+ if (kdfParams != null) {
+ LOG.log(DEBUG, "Returning cached KDFParams");
+ return kdfParams;
+ }
- if (concatKDFParamsElement == null) {
- return null;
+ String kdfAlgorithm = getAlgorithm();
+ if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_CONCATKDF.equals(kdfAlgorithm)) {
+ Element concatKDFParamsElement =
+ XMLUtils.selectXenc11Node(getElement().getFirstChild(),
+ EncryptionConstants._TAG_CONCATKDFPARAMS, 0);
+ kdfParams = new ConcatKDFParamsImpl(concatKDFParamsElement,
getBaseURI());
+ } else if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_HKDF.equals(kdfAlgorithm)) {
+ Element hkdfParamsElement =
+ XMLUtils.selectNode(getElement().getFirstChild(),
+ Constants.XML_DSIG_NS_MORE_21_04,
+ EncryptionConstants._TAG_HKDFPARAMS, 0);
+ kdfParams = new HKDFParamsImpl(hkdfParamsElement,
Constants.XML_DSIG_NS_MORE_07_05);
}
- concatKDFParams = new ConcatKDFParamsImpl(concatKDFParamsElement,
getBaseURI());
- return concatKDFParams;
+ return kdfParams;
}
- public void setConcatKDFParams(ConcatKDFParamsImpl concatKDFParams) {
- this.concatKDFParams = concatKDFParams;
- appendSelf(concatKDFParams);
- addReturnToSelf();
+ public void setKDFParams(KDFParams kdfParams) {
Review Comment:
Should you check if the KDFParams are of a supported type and throw an
Exception if not?
##########
src/main/java/org/apache/xml/security/utils/KeyUtils.java:
##########
@@ -248,7 +246,6 @@ public static int getAESKeyBitSizeForWrapAlgorithm(String
keyWrapAlg) throws XML
}
}
-
/**
* Derive a key encryption key from a shared secret and
keyDerivationParameter. Currently only the ConcatKDF is supported.
Review Comment:
The second sentence should be updated now that HKDF is also supported.
##########
src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java:
##########
@@ -243,48 +270,55 @@ public static KeyAgreementParameters
constructAgreementParameters(String agreeme
/**
* Construct a KeyDerivationParameter object from the given
keyDerivationMethod and keyBitLength
*
- * @param keyDerivationMethod element to parse
- * @param keyBitLength expected derived key length
- * @return KeyDerivationParameter object
- * @throws XMLSecurityException if the keyDerivationMethod is not supported
+ * @param keyDerivationMethod element with the key derivation method data
+ * @param keyBitLength expected derived key length
+ * @return KeyDerivationParameters data
+ * @throws XMLSecurityException if the keyDerivationMethod is not
supported or invalid parameters are provided
*/
public static KeyDerivationParameters
constructKeyDerivationParameter(KeyDerivationMethod keyDerivationMethod, int
keyBitLength) throws XMLSecurityException {
String keyDerivationAlgorithm = keyDerivationMethod.getAlgorithm();
- if
(!EncryptionConstants.ALGO_ID_KEYDERIVATION_CONCATKDF.equals(keyDerivationAlgorithm))
{
- throw new XMLEncryptionException("unknownAlgorithm",
keyDerivationAlgorithm);
- }
- ConcatKDFParamsImpl concatKDFParams = ((KeyDerivationMethodImpl)
keyDerivationMethod).getConcatKDFParams();
+ KDFParams kdfParams = keyDerivationMethod.getKDFParams();
+ if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_CONCATKDF.equals(keyDerivationAlgorithm))
{
+ if (! (kdfParams instanceof ConcatKDFParamsImpl)) {
+ throw new
XMLEncryptionException("KeyDerivation.InvalidParametersType",
keyDerivationAlgorithm, ConcatKDFParamsImpl.class.getName());
+ }
- return constructConcatKeyDerivationParameter(keyBitLength,
concatKDFParams.getDigestMethod(), concatKDFParams.getAlgorithmId(),
- concatKDFParams.getPartyUInfo(),
concatKDFParams.getPartyVInfo(),
-
concatKDFParams.getSuppPubInfo(),concatKDFParams.getSuppPrivInfo());
+ ConcatKDFParamsImpl concatKDFParams = (ConcatKDFParamsImpl)
kdfParams;
- }
+ return constructConcatKeyDerivationParameter(keyBitLength,
concatKDFParams.getDigestMethod(), concatKDFParams.getAlgorithmId(),
+ concatKDFParams.getPartyUInfo(),
concatKDFParams.getPartyVInfo(),
+ concatKDFParams.getSuppPubInfo(),
concatKDFParams.getSuppPrivInfo());
+ } else if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_HKDF.equals(keyDerivationAlgorithm))
{
+ if (! (kdfParams instanceof HKDFParamsImpl)) {
- /**
- * Construct a ConcatKeyDerivationParameter object from the key length and
digest method.
- *
- * @param keyBitLength expected derived key length
- * @param digestMethod digest method
- * @return ConcatKeyDerivationParameter object
- */
- public static ConcatKDFParams constructConcatKeyDerivationParameter(int
keyBitLength,
- String
digestMethod){
- return constructConcatKeyDerivationParameter(keyBitLength,
digestMethod, null, null, null, null, null);
+ throw new
XMLEncryptionException("KeyDerivation.InvalidParametersType",
keyDerivationAlgorithm, HKDFParamsImpl.class.getName());
+ }
+ HKDFParamsImpl hKDFParams = (HKDFParamsImpl) kdfParams;
+ return constructHKDFKeyDerivationParameter(keyBitLength,
+ hKDFParams.getPRFAlgorithm(),
+ hKDFParams.getSalt() != null ?
Base64.getDecoder().decode(hKDFParams.getSalt()) : null,
+ hKDFParams.getInfo() != null ?
Base64.getDecoder().decode(hKDFParams.getInfo()) : null);
+ }
+ throw new XMLEncryptionException("unknownAlgorithm",
keyDerivationAlgorithm);
}
/**
- * Construct a ConcatKeyDerivationParameter object from the given
parameters
+ * Construct a ConcatKeyDerivationParameter object from the given
parameters as specified in the XML Encryption 1.1
+ * and NIST SP 800-56Ar2 specifications. (In a key establishment
transaction, the participants,
+ * parties U and V, are considered to be the first and second parties)
*
* @param keyBitLength expected derived key length
- * @param digestMethod digest method
- * @param algorithmId algorithm id
- * @param partyUInfo partyUInfo
- * @param partyVInfo partyVInfo
- * @param suppPubInfo suppPubInfo
- * @param suppPrivInfo suppPrivInfo
- * @return ConcatKeyDerivationParameter object
+ * @param digestMethod digest method element identifies the digest
algorithm used by the KDF
+ * @param algorithmId algorithm id indicates how the derived keying
material will be parsed and for which
+ * algorithm(s) the derived secret keying material
will be used.
+ * @param partyUInfo partyUInfo containing public information about party
+ * @param partyVInfo partyVInfo containing public information about party
+ * @param suppPubInfo suppPubInfo An optional subfield, which could be
null that contains additional, mutually
+ * known public information
+ * @param suppPrivInfo suppPrivInfo An optional subfield, which could be
null, that contains additional, mutually
+ * known private information
+ * @return ConcatKeyDerivationParameter parameters used as input to the
Concatenation Key Derivation Function
Review Comment:
s/ConcatKeyDerivationParameter/ConcatKDFParams/
##########
src/main/java/org/apache/xml/security/encryption/XMLCipherUtil.java:
##########
@@ -140,7 +140,7 @@ public static OAEPParameterSpec constructOAEPParameters(
public static MGF1ParameterSpec constructMGF1Parameter(String
mgh1AlgorithmURI) {
LOG.log(Level.DEBUG, "Creating MGF1ParameterSpec for [{0}]",
mgh1AlgorithmURI);
if (mgh1AlgorithmURI == null || mgh1AlgorithmURI.isEmpty()) {
- LOG.log(Level.WARNING,"MGF1 algorithm URI is null or empty. Using
SHA-1 as default.");
+ LOG.log(Level.WARNING, "MGF1 algorithm URI is null or empty. Using
SHA-1 as default.");
Review Comment:
We should strongly consider updating the default to at least SHA-256. Please
file a separate issue for this.
##########
src/main/java/org/apache/xml/security/encryption/KeyDerivationMethod.java:
##########
@@ -19,10 +19,15 @@
package org.apache.xml.security.encryption;
+import org.apache.xml.security.encryption.keys.content.derivedKey.KDFParams;
+import org.apache.xml.security.exceptions.XMLSecurityException;
+
/**
- * The key derivation is to generate new cryptographic key material from
existing key material such as the shared
- * secret and any other (private or public) information. The purpose of the
key derivation is an extension of a given
- * but limited set of original key materials and to limit the use (exposure)
of such key material.
+ * The key derivation is to generate new cryptographic key material from
existing
+ * key material such as the shared secret and any other (private or public)
+ * information. The purpose of the key derivation is an extension of a given
+ * but limited set of original key materials and to limit the use (exposure)
+ * of such key material.
Review Comment:
Suggest some rewording to improve readability:
"The purpose of key derivation is to generate new cryptographic key material
from existing key material such as the shared secret and any other (private or
public) information. This class represents the KeyDerivationMethod element."
I didn't understand the second sentence.
##########
src/main/java/org/apache/xml/security/encryption/keys/content/derivedKey/KDFParams.java:
##########
@@ -0,0 +1,26 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.xml.security.encryption.keys.content.derivedKey;
+
+/**
+ * Marker interface for all KDFParams implementations
Review Comment:
There should be a period at the end of the sentence here.
##########
src/main/java/org/apache/xml/security/encryption/keys/content/derivedKey/KeyDerivationMethodImpl.java:
##########
@@ -71,29 +73,43 @@ public String getAlgorithm() {
return getLocalAttribute(EncryptionConstants._ATT_ALGORITHM);
}
- public ConcatKDFParamsImpl getConcatKDFParams() throws
XMLSecurityException {
- if (concatKDFParams != null) {
- return concatKDFParams;
- }
+ @Override
+ public KDFParams getKDFParams() throws XMLSecurityException {
- Element concatKDFParamsElement =
- XMLUtils.selectXenc11Node(getElement().getFirstChild(),
EncryptionConstants._TAG_CONCATKDFPARAMS, 0);
+ if (kdfParams != null) {
+ LOG.log(DEBUG, "Returning cached KDFParams");
+ return kdfParams;
+ }
- if (concatKDFParamsElement == null) {
- return null;
+ String kdfAlgorithm = getAlgorithm();
+ if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_CONCATKDF.equals(kdfAlgorithm)) {
+ Element concatKDFParamsElement =
+ XMLUtils.selectXenc11Node(getElement().getFirstChild(),
+ EncryptionConstants._TAG_CONCATKDFPARAMS, 0);
+ kdfParams = new ConcatKDFParamsImpl(concatKDFParamsElement,
getBaseURI());
+ } else if
(EncryptionConstants.ALGO_ID_KEYDERIVATION_HKDF.equals(kdfAlgorithm)) {
+ Element hkdfParamsElement =
+ XMLUtils.selectNode(getElement().getFirstChild(),
+ Constants.XML_DSIG_NS_MORE_21_04,
+ EncryptionConstants._TAG_HKDFPARAMS, 0);
+ kdfParams = new HKDFParamsImpl(hkdfParamsElement,
Constants.XML_DSIG_NS_MORE_07_05);
}
- concatKDFParams = new ConcatKDFParamsImpl(concatKDFParamsElement,
getBaseURI());
- return concatKDFParams;
+ return kdfParams;
Review Comment:
According to the javadoc, this should throw an XMLSecurityException if the
params are of an unknown type.
##########
src/main/java/org/apache/xml/security/utils/KeyUtils.java:
##########
@@ -258,17 +255,44 @@ public static int getAESKeyBitSizeForWrapAlgorithm(String
keyWrapAlg) throws XML
*/
public static byte[] deriveKeyEncryptionKey(byte[] sharedSecret,
KeyDerivationParameters keyDerivationParameter)
throws XMLSecurityException {
- int iKeySize = keyDerivationParameter.getKeyBitLength()/8;
+
+ if (keyDerivationParameter == null) {
+ throw new IllegalArgumentException("KeyDerivationParameter is
null");
Review Comment:
The javadoc should have an `@throws IllegalArgumentException if
keyDerivationParameter is null`
##########
src/main/java/org/apache/xml/security/encryption/params/HKDFParams.java:
##########
@@ -0,0 +1,76 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.xml.security.encryption.params;
+
+import org.apache.xml.security.signature.XMLSignature;
+import org.apache.xml.security.utils.EncryptionConstants;
+
+/**
+ * Class HMacKeyDerivationParameter (HKDF parameter) is used to specify
Review Comment:
Class name needs to be updated. Several methods need javadoc describing the
parameters or return values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@santuario.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
