- Download Diff

Summary:

8982 Support building with OpenSSL 1.1

Review Request #827 — Created Jan. 31, 2018 and submitted April 14, 2018, 9:16 a.m.

Information
Owner:	citrus
Repository:	illumos-gate
Branch:	master
Bugs:	8982
Depends On:
Commit:	c6ccd50...
Reviewers
Groups:	general
People:

Description

8982 Support building with OpenSSL 1.1

Testing Done


libkrb5/pkinit - tested certificate-based preauth/pkinit between machine and local KDC. Confirmed modified code was being used via dtrace.
libkmf - Test kmf_openssl plugin with pktool.
sendmail - tested basic TLS interop

Issues

0
0
9
0
All Issues: 9

Description	From	Last Updated
This check is somewhat bothersome -- we don't really need the openssl utility to build the gate, only the openssl ...	yuripv	Feb. 10, 2018, 10:12 a.m.
Is this strictly for version detection/printing in nightly(1) (via usr/src/Makefile)?	jbk	Feb. 21, 2018, 11:33 p.m.
Can the DSA_new() call succeed while the DSA_generate_parameters_ex() call fail? If so, this could leak memory.	jbk	April 11, 2018, 4:02 p.m.
It seems like if 'q = BN_new()' fails, g will get leaked.	jbk	April 11, 2018, 4:02 p.m.
Similar concern about leaking g as above.	jbk	April 11, 2018, 4:02 p.m.
if q = BN_new() fails, it appears g will be leaked.	jbk	April 11, 2018, 4:02 p.m.
Won't this leak rsa if it fails?	jbk	April 11, 2018, 4:11 p.m.
Won't this leak rsa and n if this fails?	jbk	April 11, 2018, 4:11 p.m.
Similar questions as above.	jbk	April 11, 2018, 4:13 p.m.

Testing Done:

	+	libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1`
	+	sendmail - tested basic TLS interop
	+	libkmf - limited testing with `pktool` so far. Need to write test code to exercise code paths.
	+
	+
	+
	+	wanboot - TBC
	+	libkrb5 - TBC
	+	libpkcs11 - TBC

This looks very interesting. I wonder though how this compares to using LibreSSL -- will we have to do separate changes to use it as well, or this makes us closer to both OpenSSL 1.1 and LibreSSL?

citrus Feb. 8, 2018, 12:06 a.m.

This change still retains compatibility with the OpenSSL 1.0 API by changing pre 1.0 calls and methods to new ones supported by both 1.0 & 1.1, or using 1.1 methods and adding a backwards-compatibility function for 1.0.
So I don't think it takes us any closer to, or further away from, LibreSSL.

Testing Done:

		libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1`
		sendmail - tested basic TLS interop
		libkmf - limited testing with `pktool` so far. Need to write test code to exercise code paths.



~		wanboot - TBC
~		libkrb5 - TBC
~		libpkcs11 - TBC
	~	libkrb5 - TBC
	~	libpkcs11 - TBC
	~
	+
	+
	+	wanboot - planned for removal under illumos 9070

Change Summary:

re-based following wanboot removal

Testing Done:

		libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1`
		sendmail - tested basic TLS interop
		libkmf - limited testing with `pktool` so far. Need to write test code to exercise code paths.



		libkrb5 - TBC
		libpkcs11 - TBC
-
-
-
-		wanboot - planned for removal under illumos 9070

Commit:

-	3b541dfc4944bbccdab2e41dd236533699a830de
+	d71b081baacf0c8879f75314606ad5dc77e59fdc

Diff:

Revision 2 (+1541 -692)

Show changes

	usr/src/Makefile.master
	usr/src/Makefile
	usr/src/cmd/sendmail/src/tls.c
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.h
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c
	usr/src/lib/libipsecutil/common/ipsec_libssl_setup.c
	usr/src/lib/libkmf/plugins/kmf_openssl/Makefile.com
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.h
	4 more

usr/src/Makefile (Diff revision 2)

Show all issues

This check is somewhat bothersome -- we don't really need the openssl utility to build the gate, only the openssl libs? I understand that I'm nitpicking here, but still..

citrus Feb. 10, 2018, 10:11 a.m.

Good point, it should not abort if the openssl utility is not found - I will remove the exit line.
It's still useful to have the openssl version and API level in the mail_msg since for a while at least people will be using different versions and maybe different API levels within OpenSSL 1.1

Change Summary:

Do not abort if no openssl utility found (only libraries are required to build)

Commit:

-	d71b081baacf0c8879f75314606ad5dc77e59fdc
+	b4a8eefc0ec061f57d5b4156d19809108e86d456

Diff:

Revision 3 (+1540 -692)

Show changes

	usr/src/Makefile.master
	usr/src/Makefile
	usr/src/cmd/sendmail/src/tls.c
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.h
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c
	usr/src/lib/libipsecutil/common/ipsec_libssl_setup.c
	usr/src/lib/libkmf/plugins/kmf_openssl/Makefile.com
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.h
	4 more

Change Summary:

Updated information following additional testing.

Testing Done:

~		libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1`
	~	libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1` which is the only gate consumer of the modified function.
		sendmail - tested basic TLS interop
~		libkmf - limited testing with `pktool` so far. Need to write test code to exercise code paths.
~
~
	~	libkmf - Test kmf_openssl plugin with `pktool`.
	~	libkrb5/pkinit - tested certificate-based preauth/pkinit between machine and local KDC. Confirmed modified code was being used via dtrace.
	~	pkcs11_tpm - Initialised TPM, created CSR, certificate, keypair using pktool via pkcs11 && pkcs_tpm.
-
-		libkrb5 - TBC
-		libpkcs11 - TBC

Bugs:

+	8982

Aside from the OPENSSL macro question, there was nothing obvious from a first glance. I'll plan to look over everything some more a bit later as well, so there might be more questions later.

usr/src/Makefile.master (Diff revision 3)

Show all issues

Is this strictly for version detection/printing in nightly(1) (via usr/src/Makefile)?

citrus Feb. 21, 2018, 11:33 p.m.

Purely for including the version and API level in the mail_msg as an indication of the build environment. It can be overridden through the .env file if in a different place.
The openssl binary is not mandatory for building, if it isn't found then there's just a message to that effect.

Change Summary:

Remove changes to pkcs11_tpm since '9156 Remove openssl dependency from pkcs11_tpm'
Remove changes to libipsecutil since it is being dealt with under https://illumos.org/rb/r/966/

Testing Done:

~		libipsecutil - tested code path with stand-alone test utilty. Also confirmed `ikeadm dump p1` which is the only gate consumer of the modified function.
~		sendmail - tested basic TLS interop
~		libkmf - Test kmf_openssl plugin with `pktool`.
	~	libkrb5/pkinit - tested certificate-based preauth/pkinit between machine and local KDC. Confirmed modified code was being used via dtrace.
	~	libkmf - Test kmf_openssl plugin with `pktool`.
	~	sendmail - tested basic TLS interop
-		libkrb5/pkinit - tested certificate-based preauth/pkinit between machine and local KDC. Confirmed modified code was being used via dtrace.
-		pkcs11_tpm - Initialised TPM, created CSR, certificate, keypair using pktool via pkcs11 && pkcs_tpm.

Commit:

-	b4a8eefc0ec061f57d5b4156d19809108e86d456
+	f313354d230cae3b83786f76547b21738342f41a

Diff:

Revision 4 (+1437 -679)

Show changes

	usr/src/Makefile.master
	usr/src/Makefile
	usr/src/cmd/sendmail/src/tls.c
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.h
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c
	usr/src/lib/libkmf/plugins/kmf_openssl/Makefile.com
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.h
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.c
	2 more

Ship it!

```
Ship It!
```

usr/src/cmd/sendmail/src/tls.c (Diff revision 4)

Show all issues

Can the DSA_new() call succeed while the DSA_generate_parameters_ex() call fail?  If so, this could leak memory.

usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c (Diff revision 4)
Show all issues
```
It seems like if 'q = BN_new()' fails, g will get leaked.
```
usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c (Diff revision 4)
Show all issues
```
Similar concern about leaking g as above.
```
usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c (Diff revision 4)
Show all issues
```
if q = BN_new() fails, it appears g will be leaked.
```
usr/src/lib/libkmf/plugins/kmf_openssl/common/openssl_spi.c (Diff revision 4)
Show all issues
```
Won't this leak rsa if it fails?
```
1. citrus April 11, 2018, 4:17 p.m.
  No worse than the original code, but I will fix this.
usr/src/lib/libkmf/plugins/kmf_openssl/common/openssl_spi.c (Diff revision 4)
Show all issues
```
Won't this leak rsa and n if this fails?
```
usr/src/lib/libkmf/plugins/kmf_openssl/common/openssl_spi.c (Diff revision 4)
Show all issues
```
Similar questions as above.
```

Thanks for the reviews, I've will push an updated patch-set once I've re-tested.

Commit:

-	f313354d230cae3b83786f76547b21738342f41a
+	c6ccd5050eaab09772a05d0430122230bb0338b4

Diff:

Revision 5 (+1497 -715)

Show changes

	usr/src/Makefile.master
	usr/src/Makefile
	usr/src/cmd/sendmail/src/tls.c
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.h
	usr/src/lib/krb5/plugins/preauth/pkinit/pkinit_crypto_openssl.c
	usr/src/lib/libkmf/plugins/kmf_openssl/Makefile.com
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.h
	usr/src/lib/libkmf/plugins/kmf_openssl/common/compat.c
	2 more

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

8982 Support building with OpenSSL 1.1

Testing Done:

Testing Done:

Change Summary:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Testing Done:

Bugs:

Change Summary:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Status: Closed (submitted)