Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add brainpool curves for EC key generation/cypher (main branch) #293

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Add brainpool curves for EC key generation/cypher (main branch) #293

merged 2 commits into from
Mar 21, 2024

Conversation

yklymenko
Copy link
Contributor

The protocol for the communication between energy market participants in Germany enforce to use Brainpool P256r1 elliptic curve for signing and encryption of the soap parts (headers and attachments) .

This is a part (BDEW-profile) of the bigger AS4 Standart for the communication.

It would be nice to introduce this curves in santuario-xml-security to be able to use it with wss4j library.

The same as #292, but for main branch

@yklymenko yklymenko changed the title Add brainpool curves Add brainpool curves for EC key generation/cypher (main branch) Mar 15, 2024
@yklymenko yklymenko mentioned this pull request Mar 15, 2024
Closed
@coheigea
Copy link
Contributor

Looks fine to me. @seanjmullan can you review please?

@seanjmullan
Copy link
Member

Looks fine. It would be nice if there was also a test using the javax.xml.crypto.dsig API.

@yklymenko
Copy link
Contributor Author

Looks fine. It would be nice if there was also a test using the javax.xml.crypto.dsig API.

@seanjmullan you mean, something like org.apache.xml.security.test.dom.signature.EDDSASignatureTest ? I could try in the next days create such test

@seanjmullan
Copy link
Member

Looks fine. It would be nice if there was also a test using the javax.xml.crypto.dsig API.

@seanjmullan you mean, something like org.apache.xml.security.test.dom.signature.EDDSASignatureTest ? I could try in the next days create such test

Yes, something like that would be great.

@yklymenko
Copy link
Contributor Author

Looks fine. It would be nice if there was also a test using the javax.xml.crypto.dsig API.

@seanjmullan you mean, something like org.apache.xml.security.test.dom.signature.EDDSASignatureTest ? I could try in the next days create such test

Yes, something like that would be great.

@seanjmullan I've added the test. I have some problem to run org.apache.xml.security.utils.KeyUtilsTest unter jdk17 with profile bouncecastle. The "DH" Case for generateEphemeralDHKeyPair seems to be broken. org.apache.xml.security.utils.KeyUtils.KeyType#DH declaration use 1.2.840.113549.1.3.1 oid, but 1.2.840.10046.2.1 seems to be the correct one (http://oid-info.com/get/1.2.840.10046.2.1). Or may be I'm wrong?

@jrihtarsic
Copy link
Contributor

@yklymenko good catch: Looks like SunJCE returns different OID for generated (and then encoded) keys with algorithm name 'DH' than the BouncyCastle:

The result for test below is :

Expected :1.2.840.113549.1.3.1
Actual :1.2.840.10046.2.1

@Test
 void generateEphemeralDHKeyPairComparison() throws Exception {
     // gen DH key pair with SunJCE
     KeyPairGenerator kpgSunJCE = KeyPairGenerator.getInstance("DH");
     KeyPair keyPairSun = kpgSunJCE.generateKeyPair();

     // gen DH key pair with BC
     KeyPairGenerator kpgBC = KeyPairGenerator.getInstance("DH", new org.bouncycastle.jce.provider.BouncyCastleProvider());
     KeyPair keyPairBC = kpgBC.generateKeyPair();

     String keyOidSunJCE = DERDecoderUtils.getAlgorithmIdFromPublicKey(keyPairSun.getPublic());
     String keyOidBC = DERDecoderUtils.getAlgorithmIdFromPublicKey(keyPairBC.getPublic());

     Assertions.assertEquals(keyOidSunJCE, keyOidBC);
 }

@jrihtarsic
Copy link
Contributor

My suggestion is
To remove
DH("DH", "PKCS #3", KeyAlgorithmType.DH, "1.2.840.113549.1.3.1"),
From KeyType

The Diffie-Hellman key agreement using RSA keys is gradually becoming obsolete and currently, it is not supported by the xmlsec key agreement method implementation. I included it there primarily for the sake of completeness, anticipating that someone might (but not very likely) add support for Diffie-Hellman in the future.

Beside CodeQL marks it as potentially unsecure:
image

@yklymenko
Copy link
Contributor Author

My suggestion is To remove DH("DH", "PKCS #3", KeyAlgorithmType.DH, "1.2.840.113549.1.3.1"), From KeyType

The Diffie-Hellman key agreement using RSA keys is gradually becoming obsolete and currently, it is not supported by the xmlsec key agreement method implementation. I included it there primarily for the sake of completeness, anticipating that someone might (but not very likely) add support for Diffie-Hellman in the future.

Beside CodeQL marks it as potentially unsecure: image

In general, I've looked in BC how they decide, which oid should be used. They have both oid's as aliases and select one of them depending on configuration in org.bouncycastle.jcajce.provider.asymmetric.dh.BCDHPublicKey#getEncoded (see both return statements)
Sure, I can delete two or three lines to make it green again, but I'm not sure, that this should be a part of this PR

@jrihtarsic jrihtarsic mentioned this pull request Mar 21, 2024
Merged
@jrihtarsic
Copy link
Contributor

@yklymenko, you have a good point.
To make it transparent I created now the SANTUARIO-613 and the PR #297

@coheigea coheigea merged commit e0eeef3 into apache:main Mar 21, 2024
3 checks passed
coheigea pushed a commit that referenced this pull request Mar 21, 2024
@yklymenko @coheigea
Add brainpool curves for EC key generation/cypher (main branch) (#293)
3b1fab6 
* Add brainpool curves

* Add test for brainpool signature

---------

Co-authored-by: Yevgen Klymenko <coneva@klymenko.de>
@seanjmullan
Copy link
Member

Looks fine. It would be nice if there was also a test using the javax.xml.crypto.dsig API.

@seanjmullan you mean, something like org.apache.xml.security.test.dom.signature.EDDSASignatureTest ? I could try in the next days create such test

Yes, something like that would be great.

@seanjmullan I've added the test. I have some problem to run org.apache.xml.security.utils.KeyUtilsTest unter jdk17 with profile bouncecastle. The "DH" Case for generateEphemeralDHKeyPair seems to be broken. org.apache.xml.security.utils.KeyUtils.KeyType#DH declaration use 1.2.840.113549.1.3.1 oid, but 1.2.840.10046.2.1 seems to be the correct one (http://oid-info.com/get/1.2.840.10046.2.1). Or may be I'm wrong?

This is still using the Santuario API (org.apache.xml.security). Sorry, I missed the package name of the test you said above when I said that would be great.

What I meant is something like test/java/org/apache/xml/security/test/javax/xml/crypto/dsig/XMLSignatureEdDSATest.java

@yklymenko yklymenko deleted the main_brainpool_curves branch March 22, 2024 06:37
@jrihtarsic
Copy link
Contributor

@yklymenko @seanjmullan
I can do that. I have already internal task to do verify it for eDelivery AS4 profile and I can
implement also the required tests to santuario. I created a task:
https://issues.apache.org/jira/browse/SANTUARIO-614
And I will provide the PR today for it.

@jrihtarsic jrihtarsic mentioned this pull request Mar 22, 2024
Merged
@phax phax mentioned this pull request Apr 11, 2024
Closed
@seanjmullan
Copy link
Member

seanjmullan commented Apr 25, 2024 •
edited
Loading

It would be nice to define the curves in one place. Right now they are duplicated in DOMKeyValue, ECKeyValue, and ECDSAUtils. If you have time, please consider it.

seanjmullan
seanjmullan reviewed Apr 25, 2024
View reviewed changes
src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java


/**
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a description of the test.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
}

/**
* Test that encrypt and decrypt using ECDH-ES for key encryption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove "that", add period at end of sentence.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
/**
* Test that encrypt and decrypt using ECDH-ES for key encryption
* <p/>
* @throws Exception Thrown when there is any problem in signing or verification
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "... any problem in encryption or decryption"?

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
names = {"BRAINPOOLP256R1", "BRAINPOOLP384R1", "BRAINPOOLP512R1"})
void testAES128ElementEcdhEsKWCipher(KeyUtils.KeyType keyType) throws Exception {
Assumptions.assumeTrue(bcInstalled);
// Skip test for IBM JDK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove "IBM JDK" and say algorithms ... that are required are not supported.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
String source = null;
String target = null;

source = toString(d);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can combine lines 163 and 166.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
Files.write(Paths.get("target","test-enc-"+keyType.name()+".xml"), toString(ed).getBytes());

//decrypt
ee = (Element) ed.getElementsByTagName("xenc:EncryptedData").item(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can combine lines 162 and 213.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
cipherDecData.init(XMLCipher.DECRYPT_MODE, null);
cipherDecData.setKEK(privRecipientKey);
cipherDecData.setSecureValidation(true);
dd = cipherDecData.doFinal(ed, ee);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can combine lines 160 and 218.

src/test/java/org/apache/xml/security/test/dom/encryption/XMLCipherBrainpoolTest.java
}
builderKeyInfo.add(encryptedKey);

ed = cipherEncData.doFinal(d, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can combine lines 159 and 208.

src/test/java/org/apache/xml/security/test/dom/signature/ECBrainpoolSignatureTest.java
class ECBrainpoolSignatureTest {

private static final String ECDSA_JKS =
"src/test/resources/org/apache/xml/security/samples/input/ecbrainpool.jks";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create this KeyStore as part of the test? We should stop storing binary files in the repository. I know that there are already existing ones, but I would like to stop doing that going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanjmullan seanjmullan seanjmullan left review comments

@coheigea coheigea coheigea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yklymenko @coheigea @seanjmullan @jrihtarsic