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

[FIXED JENKINS-39738] - Enable aes192ctr and aes256ctr ciphers if JVM supports them #14

Merged
merged 3 commits into from Apr 25, 2017

Conversation

oleg-nenashev
Copy link
Member

If the JVM supports unlimited-strength encryption, we can enable more ciphers.
And the new SSHD core version provides good API for it.

CBC ciphers won't be added due to https://www.kb.cert.org/vuls/id/958563 . So it is not exactly what has been requested in JENKINS-39738 though I think it is a reasonable fix.

@reviewbybees @jglick + @johnou @chillum since it was raised in #5 (comment)

… supports them

If the JVM supports unlimited-strength encryption, we can enable more ciphers.
And the new SSHD core version provides good API for it.

CBC ciphers won't be added due to https://www.kb.cert.org/vuls/id/958563
@ghost
Copy link

ghost commented Apr 24, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

}
} else {
// We cannot determine if the cipher is supported, but the default configuration lists only Built-in ciphers.
// So somebody explicitly added it on his own risk.
Copy link
Member

Choose a reason for hiding this comment

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

if we log something here at info level would it be too chatty?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should happen once during the SSHD startup, so I do not think it is a big deal. But the problem is that these errors will appear in Stock Java builds, hence downgrading to FINE is probably a good approach

Copy link
Member

Choose a reason for hiding this comment

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

fine sgtm

@johnou
Copy link
Member

johnou commented Apr 24, 2017

Thanks for following up with this @oleg-nenashev!

if (c.isSupported()) {
activatedCiphers.add(cipher);
} else {
LOGGER.log(Level.FINE, "Discovered unsupported built-in Cipher: {0}. It won't be enabled", c);
Copy link
Member

Choose a reason for hiding this comment

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

Recommend will not or won’t as message formats get confused by '.

@oleg-nenashev
Copy link
Member Author

@reviewbybees done since the last change is trivial.

@oleg-nenashev oleg-nenashev merged commit bb69634 into jenkinsci:master Apr 25, 2017
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.

None yet

4 participants