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

Don't continue with checksum check if openssl sha512 is unavailable #3516

Merged
merged 2 commits into from
Aug 30, 2018

Conversation

Bronsa
Copy link
Contributor

@Bronsa Bronsa commented Aug 24, 2018

On some macOS versions openssl doesn't come with sha256 support by default:

[~] openssl sha512 < /dev/null
openssl:Error: 'sha512' is an invalid command.

If that's the case the installation script currently fails with a checksum mismatch, which is not desiderable

On some macOS versions openssl doesn't come with sha256 support by default:
```
[~] openssl sha512 < /dev/null
openssl:Error: 'sha512' is an invalid command.
```

If that's the case the installation script currently fails with a checksum mismatch, which is not desiderable
@rjbou
Copy link
Collaborator

rjbou commented Aug 28, 2018

It would be better to rely on return code than grep an error message (they depends of the os, the locale, etc.). For example, here I don't have Error, which makes the condition true even if it fails:

$ openssl sha522 2>&1 < /dev/null 
Invalid command 'sha522'; type "help" for a list.
$ openssl sha522 > /dev/null 2>&1 < /dev/null 
$ echo $?
1
$ openssl sha512 > /dev/null 2>&1 < /dev/null 
$ echo $?
0

Thanks for the contribution!

@Bronsa
Copy link
Contributor Author

Bronsa commented Aug 29, 2018

Good point! how does it look now?

@Bronsa
Copy link
Contributor Author

Bronsa commented Aug 29, 2018

Actually, we've just found yet another weirdness with macOS: on some systems a missing command still returns with a code of 0 :/

[~]> openssl sha513 2>&1 < /dev/null
openssl:Error: 'sha513' is an invalid command.

Standard commands
asn1parse         ca                certhash          ciphers
crl               crl2pkcs7         dgst              dh
dhparam           dsa               dsaparam          ec
ecparam           enc               engine            errstr
gendh             gendsa            genpkey           genrsa
nseq              ocsp              passwd            pkcs12
pkcs7             pkcs8             pkey              pkeyparam
pkeyutl           prime             rand              req
rsa               rsautl            s_client          s_server
s_time            sess_id           smime             speed
spkac             ts                verify            version
x509

Message Digest commands (see the `dgst' command for more details)
gost-mac          md4               md5               md_gost94
ripemd160         sha               sha1              sha224
sha256            sha384            sha512            streebog256
streebog512       whirlpool

Cipher commands (see the `enc' command for more details)
aes-128-cbc       aes-128-ecb       aes-192-cbc       aes-192-ecb
aes-256-cbc       aes-256-ecb       base64            bf
bf-cbc            bf-cfb            bf-ecb            bf-ofb
camellia-128-cbc  camellia-128-ecb  camellia-192-cbc  camellia-192-ecb
camellia-256-cbc  camellia-256-ecb  cast              cast-cbc
cast5-cbc         cast5-cfb         cast5-ecb         cast5-ofb
chacha            des               des-cbc           des-cfb
des-ecb           des-ede           des-ede-cbc       des-ede-cfb
des-ede-ofb       des-ede3          des-ede3-cbc      des-ede3-cfb
des-ede3-ofb      des-ofb           des3              desx
rc2               rc2-40-cbc        rc2-64-cbc        rc2-cbc
rc2-cfb           rc2-ecb           rc2-ofb           rc4
rc4-40

[~]> echo $?
0

I've made a patch that checks directly the return value, this seems to be the most secure approach

@rjbou
Copy link
Collaborator

rjbou commented Aug 30, 2018

erf, If we can't rely on return code anymore...
It's ok for me like that. I'll squash and merge the PR.
Thanks!!

@rjbou rjbou changed the title Don't continue with checksum check if openssl sha256 is unavailable Don't continue with checksum check if openssl sha512 is unavailable Aug 30, 2018
@rjbou rjbou merged commit 22b6553 into ocaml:master Aug 30, 2018
@Bronsa
Copy link
Contributor Author

Bronsa commented Aug 30, 2018

Awesome, thanks!

rjbou pushed a commit to rjbou/opam that referenced this pull request Sep 25, 2018
…caml#3516)

Don't continue with checksum check if openssl sha512 is unavailable
rjbou pushed a commit to rjbou/opam that referenced this pull request Oct 17, 2018
…caml#3516)

Don't continue with checksum check if openssl sha512 is unavailable
dra27 pushed a commit to dra27/opam that referenced this pull request Jul 12, 2019
…caml#3516)

Don't continue with checksum check if openssl sha512 is unavailable
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.

2 participants