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.

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

[JENKINS-62552] Use standard crypto APIs #45

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Conversation

jvz
Copy link
Member

@jvz jvz commented Jun 3, 2020

This deprecates the included implementations of block ciphers in preference of the ones bundled with the JDK.

Signed-off-by: Matt Sicker boards@gmail.com

@jeffret-b @daniel-beck @Wadeck

This deprecates the included implementations of block ciphers in preference of the ones bundled with the JDK.

Signed-off-by: Matt Sicker <boards@gmail.com>
@jvz jvz closed this Jun 3, 2020
@jvz jvz reopened this Jun 3, 2020
@jvz
Copy link
Member Author

jvz commented Jun 3, 2020

Alright, there we go, passing CI.

/**
* BlockCipher that delegates cryptographic operations to {@code javax.crypt.Cipher}.
*/
public class JreCipherWrapper implements BlockCipher {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a new class a unit test is recommended

jvz added 2 commits June 9, 2020 11:43
Signed-off-by: Matt Sicker <boards@gmail.com>
@jvz
Copy link
Member Author

jvz commented Jun 9, 2020

So far I've verified this works with my remote Debian machine. I'm trying it out now with Microsoft, Windows Server, version 1809 Datacenter Core for Containers, Server Core, x64 built on 20200512 using OpenSSH from this distribution since I'm not much of a Windows admin (I installed openjdk, maven, git, and sshd all via Chocolatey).

@jvz
Copy link
Member Author

jvz commented Jun 9, 2020

I should note that I'm using an Ed25519 key for authentication in both operating systems (the same key actually, just different usernames).

@jvz
Copy link
Member Author

jvz commented Jun 9, 2020

And the Windows/OpenSSH method also seems to work fine.

@jvz jvz requested a review from kuisathaverat June 16, 2020 15:21
try {
cipher.init(mode, new SecretKeySpec(key, algorithm), parameterSpec);
} catch (InvalidKeyException | InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException(e);

Choose a reason for hiding this comment

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

Would it make more sense to let the actual exceptions bubble up?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're checked exceptions (all subclassing GeneralSecurityException). :/

Choose a reason for hiding this comment

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

I was thinking they could just be added here, but it doesn't look like it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, not without changing the API, and the interface (and deprecated code) is copied from Bouncycastle as it is.

Copy link

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

Looks good. It will be better to switch to more standardized implementations and move away from trilead. These updates look like they should all work.

I did a little sanity testing, just to make sure it continues working in my environment. That worked fine.

@jvz
Copy link
Member Author

jvz commented Jun 23, 2020

@kuisathaverat can I get a re-review?

@Wadeck
Copy link

Wadeck commented Jul 14, 2020

@kuisathaverat Any news? ^^ (we are still keeping track of this issue in our board ;) )

Copy link
Contributor

@kuisathaverat kuisathaverat left a comment

Choose a reason for hiding this comment

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

tested manually no issues

@kuisathaverat kuisathaverat merged commit 4aada2d into jenkinsci:master Jul 14, 2020
@jvz jvz deleted the aes branch July 14, 2020 18:20
@jvz
Copy link
Member Author

jvz commented Jul 14, 2020

Thanks!

kuisathaverat added a commit to kuisathaverat/trilead-ssh2 that referenced this pull request Oct 12, 2020
kuisathaverat added a commit that referenced this pull request Oct 23, 2020
* Revert "[JENKINS-62552] Use standard crypto APIs (#45)"

This reverts commit 4aada2d.

* fix: remove JreCipherWrapperTest

* feat: add BlockCipher tests

* chore: bump junit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants