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

Solve SHA256 keytool of java11 version issue(#57301) #57302

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@alvarolmedo
Copy link

commented Jun 3, 2019

SUMMARY

keytool shows SHA1 or SHA256 (depends java version). To solve this list all hashes with verbose option and re.match by "SHA1:" string.
Fixes #57301

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

java_keystore module

ADDITIONAL INFORMATION
>>> stored_certificate_fingerprint_out=os.popen('/usr/lib/jvm/java-1.11.0-openjdk-amd64/bin/keytool -list -alias test-ssl -keystore keystore.p12 -storepass changeit -v').read()
>>> print(stored_certificate_fingerprint_out)
Nombre de Alias: test-ssl
Fecha de Creación: 3 jun. 2019
Tipo de Entrada: PrivateKeyEntry
Longitud de la Cadena de Certificado: 1
Certificado[1]:
Propietario: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Emisor: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Número de serie: 92e27965545459c6
Válido desde: Sun Jun 02 13:12:07 CEST 2019 hasta: Wed May 30 13:12:07 CEST 2029
Huellas digitales del certificado:
	 SHA1: 88:5E:0C:8E:00:D1:81:B4:12:2F:FF:28:DC:BD:77:39:8D:A5:F3:91
	 SHA256: CC:2F:2A:B6:EA:CE:86:66:22:91:84:68:9C:52:DF:6B:AF:B5:48:51:2A:0F:EC:F3:F5:E9:F7:48:3A:CF:F7:36
Nombre del algoritmo de firma: SHA256withRSA
Algoritmo de clave pública de asunto: Clave RSA de 1024 bits
Versión: 1

>>> stored_certificate_match = re.search(r"SHA1: ([\w:]+)", stored_certificate_fingerprint_out)
>>> print(stored_certificate_match.group(1))
88:5E:0C:8E:00:D1:81:B4:12:2F:FF:28:DC:BD:77:39:8D:A5:F3:91

The actual version show a wrong fingerprint to compare with a openssl sha1:

>>> stored_certificate_fingerprint_out_old=os.popen('/usr/lib/jvm/java-1.11.0-openjdk-amd64/bin/keytool -list -alias test-ssl -keystore keystore.p12 -storepass changeit').read()
>>> print(stored_certificate_fingerprint_out_old)
test-ssl, 3 jun. 2019, PrivateKeyEntry, 
Huella de certificado (SHA-256): CC:2F:2A:B6:EA:CE:86:66:22:91:84:68:9C:52:DF:6B:AF:B5:48:51:2A:0F:EC:F3:F5:E9:F7:48:3A:CF:F7:36
>>> stored_certificate_match = re.search(r": ([\w:]+)", stored_certificate_fingerprint_out_old)
>>> print(stored_certificate_match.group(1))
CC:2F:2A:B6:EA:CE:86:66:22:91:84:68:9C:52:DF:6B:AF:B5:48:51:2A:0F:EC:F3:F5:E9:F7:48:3A:CF:F7:36
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/system/java_keystore.py:140:161: E501 line too long (174 > 160 characters)

click here for bot help

@@ -136,7 +136,9 @@ def read_certificate_fingerprint(module, openssl_bin, certificate_path):


def read_stored_certificate_fingerprint(module, keytool_bin, alias, keystore_path, keystore_password):
stored_certificate_fingerprint_cmd = "%s -list -alias '%s' -keystore '%s' -storepass '%s'" % (keytool_bin, alias, keystore_path, keystore_password)
grep_bin = module.get_bin_path('grep', True)
stored_certificate_fingerprint_cmd = "%s -list -alias '%s' -keystore '%s' -storepass '%s' -v | %s SHA1" % (

This comment has been minimized.

Copy link
@Mogztter

Mogztter Jun 4, 2019

Contributor

I think it would be more portable to update the regular expression below rather than using grep

This comment has been minimized.

Copy link
@alvarolmedo

alvarolmedo Jun 5, 2019

Author

I take this way in order to preserve the unit test. I'll take a look changing the regexp.

@Mogztter

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Thanks @alvarolmedo

Is it working with Java 1.8 ?
Could you please post the output of a given keystore with Java 8, 9, 10, 11, 12 ?
I just want to make sure that we do not introduce a regression 😉

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

Sure, it works with Java 1.8. Java 9 and 10, don't have support, so IMHO, it's not important. Sorry for the spanish output, but you can check that execute keytool with -v get similar output (MD5 fingerprint it's the difference, only available with java8):

$ /usr/lib/jvm/java-1.11.0-openjdk-amd64/bin/keytool -list -alias test-ssl -keystore keystore.p12 -storepass changeit -v
Nombre de Alias: test-ssl
Fecha de Creación: 5 jun. 2019
Tipo de Entrada: PrivateKeyEntry
Longitud de la Cadena de Certificado: 1
Certificado[1]:
Propietario: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Emisor: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Número de serie: 92e27965545459c6
Válido desde: Sun Jun 02 13:12:07 CEST 2019 hasta: Wed May 30 13:12:07 CEST 2029
Huellas digitales del certificado:
	 SHA1: 88:5E:0C:8E:00:D1:81:B4:12:2F:FF:28:DC:BD:77:39:8D:A5:F3:91
	 SHA256: CC:2F:2A:B6:EA:CE:86:66:22:91:84:68:9C:52:DF:6B:AF:B5:48:51:2A:0F:EC:F3:F5:E9:F7:48:3A:CF:F7:36
Nombre del algoritmo de firma: SHA256withRSA
Algoritmo de clave pública de asunto: Clave RSA de 1024 bits
Versión: 1
-----
$ /usr/lib/jvm/java-1.8.0-openjdk-amd64/bin/keytool -list -alias test-ssl -keystore keystore.p12 -storepass changeit -v
Nombre de Alias: test-ssl
Fecha de Creación: 05-jun-2019
Tipo de Entrada: PrivateKeyEntry
Longitud de la Cadena de Certificado: 1
Certificado[1]:
Propietario: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Emisor: CN=localhost, O=Default Company Ltd, L=Default City, C=ES
Número de serie: 92e27965545459c6
Válido desde: Sun Jun 02 13:12:07 CEST 2019 hasta: Wed May 30 13:12:07 CEST 2029
Huellas digitales del certificado:
	 MD5: 3F:1C:D8:CD:47:23:04:54:73:48:03:66:A8:57:D2:DF
	 SHA1: 88:5E:0C:8E:00:D1:81:B4:12:2F:FF:28:DC:BD:77:39:8D:A5:F3:91
	 SHA256: CC:2F:2A:B6:EA:CE:86:66:22:91:84:68:9C:52:DF:6B:AF:B5:48:51:2A:0F:EC:F3:F5:E9:F7:48:3A:CF:F7:36
Nombre del algoritmo de firma: SHA256withRSA
Algoritmo de clave pública de asunto: Clave RSA de 1024 bits
Versión: 1
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/system/java_keystore.py:139:41: too-many-format-args Too many arguments for format string
lib/ansible/modules/system/java_keystore.py:140:62: undefined-variable Undefined variable 'grep_bin'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 5, 2019

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@Mogztter the unit tests fails at this point (with regexp modified and not using grep).
could you help me with unit tests?
I am not pretty sure if some change more is needed (nope imho, but tests fails...)
[I was in this point yesterday, and I decided the grep_bin option to solve it]

@Mogztter

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@alvarolmedo Sure, the unit tests are using mocks so you will need to update theses lines:

self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''), (0, 'foo: wxyz:9876:stuv', '')]

It's order based, in the above example the first command executed in the program will exit with a return code 0, output foo=abcd:1234:efgh on stdout and nothing on stderr.

Does it make sense ?

You could add a test with a Java 8 output and a Java 11 output (it could be useful to avoid regression).

Java 9 and 10, don't have support, so IMHO, it's not important

If the output is similar in Java 8 and Java 11 I don't think it will be any different in Java 9 and Java 10.

@ansibot ansibot added core_review and removed needs_revision labels Jun 5, 2019

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@Mogztter thanks for your explanation.
The tests are java8 and java11 compliant right now.

@Mogztter

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Awesome, it looks good!
I'm not sure about the Ansible policy but could you please squash your commits ?
Apart from that 👍

@alvarolmedo alvarolmedo force-pushed the alvarolmedo:devel branch from cf0dcac to 8d7774a Jun 5, 2019

@alvarolmedo alvarolmedo force-pushed the alvarolmedo:devel branch from 8d7774a to 0591dd9 Jun 5, 2019

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@Mogztter thanks for your help. :)
Squashing done!

@ansibot ansibot added the small_patch label Jun 5, 2019

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

so, what's the next step? :)

@Mogztter

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

I guess a core contributor will review it and hopefully merge it 😃

@Mogztter

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

shipit

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

OK, thank you :) 👍

@jmooo

This comment has been minimized.

Copy link

commented Jun 18, 2019

It looks like this merge forces the use of (deprecated) SHA-1 instead of trying SHA-256 first, and falling back on SHA-1 if the 256 hash doesn't exist? Specifically the below line that feeds the -sha1 flag to openssl for comparison would also need to be updated

https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/system/java_keystore.py#L119

@ansibot ansibot added the stale_ci label Jun 18, 2019

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

Hi @jmooo the line 119 isn't changed in this merge, so this merge don't force the use of SHA-1.
The fingerprint with openssl is showed in SHA-1 by default [1] (the param "-sha1" in L119 ensures this digest): " If not specified then SHA1 is used with -fingerprint or the default digest for the signing algorithm is used, typically SHA256."

$ openssl x509 -in cert.pem -noout -fingerprint
SHA1 Fingerprint=9A:82:D0:7A:87:64:BA:F1:23:02:98:25:86:F7:AA:88:54:1B:0A:88

Anyway, this PR fixes the output of the other tool used: keytool (java). The keytool used show the digest in the SHA-1 format, but in the last release (java 11) the keytool change the output by default to SHA256.[2]
This merge request change the execution of keytool, if keytool is executed with verbosity (-v) all digest formats are showed: sha-1, sha-256 and md5. At this point we can search the SHA-1 digest in the output.
[1] https://www.openssl.org/docs/manmaster/man1/x509.html
[2] https://docs.oracle.com/en/java/javase/11/tools/keytool.html

@jmooo

This comment has been minimized.

Copy link

commented Jun 18, 2019

Right, specifically I was curious if we should change that line from -sha1 to -sha256, since SHA-256 is the new standard, and appears in at least Java 8 thru Java 12. We'd still need your verbose search option, but just have it search for SHA256 instead of SHA1

Searching through the Oracle docs, md5 was dropped and sha256 became the default in Java 9. So for anyone above Java 8 an immediate fix is change -sha1 to -sha256, with no further code changes, and Ansible will report the changed: result correctly

current_certificate_fingerprint_cmd = "%s x509 -noout -in %s -fingerprint -sha1" % (openssl_bin, certificate_path)

@alvarolmedo

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

Sure, the change in line 119 works with java>=9, but these change break the compatibility with java<=8.
So, my PR tries to be compatible with all versions of java. This is the reason to use the verbose option and search for sha1 (or sha256 if you prefer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.