From 04441bbc3783722378b62daa8ce4edc6c357c083 Mon Sep 17 00:00:00 2001 From: Sao I Kuan Date: Sun, 19 Apr 2020 22:55:42 +0900 Subject: [PATCH 01/22] Add feature for loading the chained certificate into Certificate array --- ext/openssl/ossl_x509cert.c | 55 +++++++++++++++++++++++++++++++++++++ lib/openssl/x509.rb | 14 ++++++++++ 2 files changed, 69 insertions(+) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 5376bff08..991045839 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -704,6 +704,60 @@ ossl_x509_eq(VALUE self, VALUE other) return !X509_cmp(a, b) ? Qtrue : Qfalse; } +/* + * call-seq: + * cert.load_chained_cert_from_file(path) -> [certs...] + * + * Read the chained certificates from specified file path. + */ +static VALUE +ossl_x509_load_chained_cert_from_file(VALUE self, VALUE path) +{ + BIO *in; + X509 *x509; + VALUE ary = rb_ary_new(); + VALUE cert; + + in = BIO_new(BIO_s_file()); + if (in == NULL) + ossl_raise(eX509CertError, NULL); + + if (BIO_read_filename(in, StringValueCStr(path)) <= 0) + ossl_raise(eX509CertError, NULL); + + /* check the certificate format is PEM or DER */ + if ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL) { + /* case 1: certificate format is PEM */ + do { + cert = ossl_x509_new(x509); + rb_ary_push(ary, cert); + X509_free(x509); + } while ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL); + + BIO_free(in); + return ary; + } + + /* certificate format is not PEM */ + OSSL_BIO_reset(in); + if ((x509 = d2i_X509_bio(in, NULL)) != NULL) { + /* case 2: certificate format is DER */ + do { + cert = ossl_x509_new(x509); + rb_ary_push(ary, cert); + X509_free(x509); + } while ((x509 = d2i_X509_bio(in, NULL)) != NULL); + + BIO_free(in); + return ary; + } + + /* error: certificate format is not both PEM or DER */ + BIO_free(in); + ossl_raise(eX509CertError, NULL); +} + + /* * INIT */ @@ -843,4 +897,5 @@ Init_ossl_x509cert(void) rb_define_method(cX509Cert, "add_extension", ossl_x509_add_extension, 1); rb_define_method(cX509Cert, "inspect", ossl_x509_inspect, 0); rb_define_method(cX509Cert, "==", ossl_x509_eq, 1); + rb_define_method(cX509Cert, "load_chained_cert_from_file", ossl_x509_load_chained_cert_from_file, 1); } diff --git a/lib/openssl/x509.rb b/lib/openssl/x509.rb index 6771b90c1..17bb940d4 100644 --- a/lib/openssl/x509.rb +++ b/lib/openssl/x509.rb @@ -338,6 +338,20 @@ def pretty_print(q) q.text 'not_after='; q.pp self.not_after } end + + # TODO: sikuan: Load the certificates (PEM, DER, etc.) from file, using BIO + # input string (File path) + # output array (Array of Certificate objects) + def load_file(path) + return load_chained_cert_from_file(path) + end + + # TODO: sikuan: Load the certificates (PEM, DER, etc.) from BIO (various data sources) + # input cert content (DER(ASN1) or PEM) + # output array (Array of Certificate objects) + def read_certificates(cert_content) + raise NotImplementedError + end end class CRL From d5b5ccf2cbd23a92fba6bc14d35307c1b03d5027 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 11:22:15 +1200 Subject: [PATCH 02/22] Change to class method and add basic test. --- ext/openssl/ossl_x509cert.c | 95 +++++++++++++++--------- test/openssl/fixtures/pkey/empty.pem | 0 test/openssl/fixtures/pkey/fullchain.pem | 56 ++++++++++++++ test/openssl/test_x509cert.rb | 12 +++ 4 files changed, 128 insertions(+), 35 deletions(-) create mode 100644 test/openssl/fixtures/pkey/empty.pem create mode 100644 test/openssl/fixtures/pkey/fullchain.pem diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 991045839..140dac902 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -704,57 +704,81 @@ ossl_x509_eq(VALUE self, VALUE other) return !X509_cmp(a, b) ? Qtrue : Qfalse; } -/* - * call-seq: - * cert.load_chained_cert_from_file(path) -> [certs...] - * - * Read the chained certificates from specified file path. - */ static VALUE -ossl_x509_load_chained_cert_from_file(VALUE self, VALUE path) -{ - BIO *in; - X509 *x509; - VALUE ary = rb_ary_new(); - VALUE cert; +load_chained_certificates(VALUE _io) { + BIO *in = (BIO*)_io; + VALUE certificates = rb_ary_new(); - in = BIO_new(BIO_s_file()); - if (in == NULL) - ossl_raise(eX509CertError, NULL); + X509 *x509 = NULL; - if (BIO_read_filename(in, StringValueCStr(path)) <= 0) - ossl_raise(eX509CertError, NULL); - - /* check the certificate format is PEM or DER */ + // Check the certificate format is PEM or DER: + + // Case 1: certificate format is PEM: if ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL) { - /* case 1: certificate format is PEM */ do { - cert = ossl_x509_new(x509); - rb_ary_push(ary, cert); + rb_ary_push(certificates, ossl_x509_new(x509)); X509_free(x509); - } while ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL); + } while (!BIO_eof(in) && (x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL); - BIO_free(in); - return ary; + if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { + ERR_clear_error(); + } + + return certificates; } - /* certificate format is not PEM */ OSSL_BIO_reset(in); + + // Case 2: certificate format is DER: if ((x509 = d2i_X509_bio(in, NULL)) != NULL) { - /* case 2: certificate format is DER */ do { - cert = ossl_x509_new(x509); - rb_ary_push(ary, cert); + rb_ary_push(certificates, ossl_x509_new(x509)); X509_free(x509); - } while ((x509 = d2i_X509_bio(in, NULL)) != NULL); + } while (!BIO_eof(in) && (x509 = d2i_X509_bio(in, NULL)) != NULL); - BIO_free(in); - return ary; + return certificates; } - /* error: certificate format is not both PEM or DER */ + // If we got to the end of the file, we are done: + if (BIO_eof(in)) { + ERR_clear_error(); + return certificates; + } else { + // Otherwise we couldn't read everything so fail: + ossl_raise(eX509CertError, NULL); + } +} + +static VALUE +load_chained_certificates_ensure(VALUE _io) { + BIO *in = (BIO*)_io; BIO_free(in); - ossl_raise(eX509CertError, NULL); + + return Qnil; +} + +/* + * call-seq: + * OpenSSL::X509::Certificate.load(path) -> [certs...] + * + * Read the chained certificates from specified file path. + */ +static VALUE +ossl_x509_load_chained_certificates(VALUE klass, VALUE path) +{ + ERR_clear_error(); + + BIO *in = BIO_new(BIO_s_file()); + + if (in == NULL) + ossl_raise(eX509CertError, NULL); + + if (BIO_read_filename(in, StringValueCStr(path)) <= 0) { + BIO_free(in); + ossl_raise(eX509CertError, NULL); + } + + return rb_ensure(load_chained_certificates, (VALUE)in, load_chained_certificates_ensure, (VALUE)in); } @@ -866,6 +890,8 @@ Init_ossl_x509cert(void) */ cX509Cert = rb_define_class_under(mX509, "Certificate", rb_cObject); + rb_define_singleton_method(cX509Cert, "load", ossl_x509_load_chained_certificates, 1); + rb_define_alloc_func(cX509Cert, ossl_x509_alloc); rb_define_method(cX509Cert, "initialize", ossl_x509_initialize, -1); rb_define_method(cX509Cert, "initialize_copy", ossl_x509_copy, 1); @@ -897,5 +923,4 @@ Init_ossl_x509cert(void) rb_define_method(cX509Cert, "add_extension", ossl_x509_add_extension, 1); rb_define_method(cX509Cert, "inspect", ossl_x509_inspect, 0); rb_define_method(cX509Cert, "==", ossl_x509_eq, 1); - rb_define_method(cX509Cert, "load_chained_cert_from_file", ossl_x509_load_chained_cert_from_file, 1); } diff --git a/test/openssl/fixtures/pkey/empty.pem b/test/openssl/fixtures/pkey/empty.pem new file mode 100644 index 000000000..e69de29bb diff --git a/test/openssl/fixtures/pkey/fullchain.pem b/test/openssl/fixtures/pkey/fullchain.pem new file mode 100644 index 000000000..3eb6ef424 --- /dev/null +++ b/test/openssl/fixtures/pkey/fullchain.pem @@ -0,0 +1,56 @@ +-----BEGIN CERTIFICATE----- +MIIFKTCCBBGgAwIBAgISBFspP+tJfRaC6xprreB4Rp9KMA0GCSqGSIb3DQEBCwUA +MDIxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MQswCQYDVQQD +EwJSMzAeFw0yMTA0MTcwMjQzMTlaFw0yMTA3MTYwMjQzMTlaMBwxGjAYBgNVBAMT +EXd3dy5jb2Rlb3Rha3UuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC +AQEAx6h5vNPfkkrtYWxn1PWDDLRAwrGmZbkYPttjHBRSwTcd7rsIX4PcSzw9fWxm +K4vIkAYoKAElIvsSE3xRUjyzMrACfdhK5J8rG25fq94iVyoYaNBQV0WMJkO6X47s +hGeIKkK91ohR5b2tMw3/z9zELP0TVo2TPG7rYsBZm34myldqDA8yVEBEOa+Qdpda +9xewPhkkdpAU55qgWTrD21m7vGq9WpsBz4wNKnwVsaugtkRH82VPIfaL4ZI9kox6 +QoPWe/tHUBdlDkuT7ud77eLAWnC/5Clg28/9GU/Z8Nj8SrrKuXL6WUXmxxaAhWUR +Qx4VblZeuIpwd0nHyP0hz4CWKQIDAQABo4ICTTCCAkkwDgYDVR0PAQH/BAQDAgWg +MB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMB0G +A1UdDgQWBBTKiSGZuLFSIG2JPbFSZa9TxMu5WTAfBgNVHSMEGDAWgBQULrMXt1hW +y65QCUDmH6+dixTCxjBVBggrBgEFBQcBAQRJMEcwIQYIKwYBBQUHMAGGFWh0dHA6 +Ly9yMy5vLmxlbmNyLm9yZzAiBggrBgEFBQcwAoYWaHR0cDovL3IzLmkubGVuY3Iu +b3JnLzAcBgNVHREEFTATghF3d3cuY29kZW90YWt1LmNvbTBMBgNVHSAERTBDMAgG +BmeBDAECATA3BgsrBgEEAYLfEwEBATAoMCYGCCsGAQUFBwIBFhpodHRwOi8vY3Bz +LmxldHNlbmNyeXB0Lm9yZzCCAQUGCisGAQQB1nkCBAIEgfYEgfMA8QB3AJQgvB6O +1Y1siHMfgosiLA3R2k1ebE+UPWHbTi9YTaLCAAABeN3s/lgAAAQDAEgwRgIhAKFY +Q+vBe3zyeBazxp8kVN7oLvcQ6Y9PPz199tVhYnEbAiEAhU/xdbQaY/6b93h+7NTF +sPG7X4lq/3UoNgoXcAVGZgoAdgD2XJQv0XcwIhRUGAgwlFaO400TGTO/3wwvIAvM +TvFk4wAAAXjd7P5OAAAEAwBHMEUCIQDWd79+jWaGuf3acm5/yV95jL2KvzeGFfdU +HZlKIeWFmAIgDSZ6ug7AyhYNKjzFV4ZSICln+L4yI92EpOa+8gDG6/0wDQYJKoZI +hvcNAQELBQADggEBAHIhMYm06lLFmJL+cfIg5fFEmFNdHmmZn88Hypv4/MtmqTKv +5asF/z3TvhW4hX2+TY+NdcqGT7cZFo/ZF/tS6oBXPgmBYM1dEfp2FAdnGNOySC5Y +7RC4Uk9TUpP2g101YBmj6dQKQluAwIQk+gO4MSlHE0J0U/lMpjvrLWcuHbV4/xWJ +IdM+iPq8GeYt5epYmNc7XeRIgv7V3RxDQdBv2OVM5mtPVerdiO0ISrdbe5mvz2+Z +rhSg+EJNHlmMwcq5HqtMwS8M8Ax+vLmWCOkPWXhyV8wQaQcFjZJfpIGUvCnMTqsh +kSIYXq2CbSDUUFRFssNN6EdVms0KnmW3BUu0xAk= +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIEZTCCA02gAwIBAgIQQAF1BIMUpMghjISpDBbN3zANBgkqhkiG9w0BAQsFADA/ +MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT +DkRTVCBSb290IENBIFgzMB4XDTIwMTAwNzE5MjE0MFoXDTIxMDkyOTE5MjE0MFow +MjELMAkGA1UEBhMCVVMxFjAUBgNVBAoTDUxldCdzIEVuY3J5cHQxCzAJBgNVBAMT +AlIzMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAuwIVKMz2oJTTDxLs +jVWSw/iC8ZmmekKIp10mqrUrucVMsa+Oa/l1yKPXD0eUFFU1V4yeqKI5GfWCPEKp +Tm71O8Mu243AsFzzWTjn7c9p8FoLG77AlCQlh/o3cbMT5xys4Zvv2+Q7RVJFlqnB +U840yFLuta7tj95gcOKlVKu2bQ6XpUA0ayvTvGbrZjR8+muLj1cpmfgwF126cm/7 +gcWt0oZYPRfH5wm78Sv3htzB2nFd1EbjzK0lwYi8YGd1ZrPxGPeiXOZT/zqItkel +/xMY6pgJdz+dU/nPAeX1pnAXFK9jpP+Zs5Od3FOnBv5IhR2haa4ldbsTzFID9e1R +oYvbFQIDAQABo4IBaDCCAWQwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8E +BAMCAYYwSwYIKwYBBQUHAQEEPzA9MDsGCCsGAQUFBzAChi9odHRwOi8vYXBwcy5p +ZGVudHJ1c3QuY29tL3Jvb3RzL2RzdHJvb3RjYXgzLnA3YzAfBgNVHSMEGDAWgBTE +p7Gkeyxx+tvhS5B1/8QVYIWJEDBUBgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEE +AYLfEwEBATAwMC4GCCsGAQUFBwIBFiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2Vu +Y3J5cHQub3JnMDwGA1UdHwQ1MDMwMaAvoC2GK2h0dHA6Ly9jcmwuaWRlbnRydXN0 +LmNvbS9EU1RST09UQ0FYM0NSTC5jcmwwHQYDVR0OBBYEFBQusxe3WFbLrlAJQOYf +r52LFMLGMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjANBgkqhkiG9w0B +AQsFAAOCAQEA2UzgyfWEiDcx27sT4rP8i2tiEmxYt0l+PAK3qB8oYevO4C5z70kH +ejWEHx2taPDY/laBL21/WKZuNTYQHHPD5b1tXgHXbnL7KqC401dk5VvCadTQsvd8 +S8MXjohyc9z9/G2948kLjmE6Flh9dDYrVYA9x2O+hEPGOaEOa1eePynBgPayvUfL +qjBstzLhWVQLGAkXXmNs+5ZnPBxzDJOLxhF2JIbeQAcH5H0tZrUlo5ZYyOqA7s9p +O5b85o3AM/OJ+CktFBQtfvBhcJVd9wvlwPsk+uyOy2HI7mNxKKgsBTt375teA2Tw +UdHkhVNcsAKX1H7GNNLOEADksd86wuoXvg== +-----END CERTIFICATE----- \ No newline at end of file diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index 848a314c9..768b0fb08 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -276,6 +276,18 @@ def test_marshal assert_equal cert.to_der, deserialized.to_der end + def test_load_empty + empty_path = Fixtures.file_path("pkey", "empty.pem") + certificates = OpenSSL::X509::Certificate.load(empty_path) + assert_equal 0, certificates.size + end + + def test_load_fullchain + fullchain_path = Fixtures.file_path("pkey", "fullchain.pem") + certificates = OpenSSL::X509::Certificate.load(fullchain_path) + assert_equal 2, certificates.size + end + private def certificate_error_returns_false From f89efa29099117e3198ce38810b8ea0dff8618ba Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 13:55:58 +1200 Subject: [PATCH 03/22] Add more tests and fix error handling. --- ext/openssl/ossl_x509cert.c | 101 +++++++++++++++++------ test/openssl/fixtures/pkey/empty.der | 0 test/openssl/fixtures/pkey/fullchain.der | Bin 0 -> 1325 bytes test/openssl/fixtures/pkey/garbage.txt | 1 + test/openssl/test_x509cert.rb | 26 +++++- 5 files changed, 100 insertions(+), 28 deletions(-) create mode 100644 test/openssl/fixtures/pkey/empty.der create mode 100644 test/openssl/fixtures/pkey/fullchain.der create mode 100644 test/openssl/fixtures/pkey/garbage.txt diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 140dac902..6c026edf1 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -705,50 +705,99 @@ ossl_x509_eq(VALUE self, VALUE other) } static VALUE -load_chained_certificates(VALUE _io) { - BIO *in = (BIO*)_io; - VALUE certificates = rb_ary_new(); - - X509 *x509 = NULL; - - // Check the certificate format is PEM or DER: - - // Case 1: certificate format is PEM: - if ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL) { - do { - rb_ary_push(certificates, ossl_x509_new(x509)); - X509_free(x509); - } while (!BIO_eof(in) && (x509 = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL); +load_chained_certificates_PEM(BIO *in) { + X509 *x509 = PEM_read_bio_X509(in, NULL, NULL, NULL); + // If we cannot read even one certificate: + if (x509 == NULL) { + // If we cannot read one certificate because we could not read the PEM encoding: if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { ERR_clear_error(); } - return certificates; + if (ERR_peek_last_error()) + ossl_raise(eX509CertError, NULL); + else + return Qnil; } - OSSL_BIO_reset(in); + VALUE certificates = rb_ary_new(); + rb_ary_push(certificates, ossl_x509_new(x509)); - // Case 2: certificate format is DER: - if ((x509 = d2i_X509_bio(in, NULL)) != NULL) { - do { - rb_ary_push(certificates, ossl_x509_new(x509)); - X509_free(x509); - } while (!BIO_eof(in) && (x509 = d2i_X509_bio(in, NULL)) != NULL); + while ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL))) { + rb_ary_push(certificates, ossl_x509_new(x509)); + X509_free(x509); + } + + // We tried to read one more certificate but could not read start line: + if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { + // This is not an error, it means we are finished: + ERR_clear_error(); return certificates; } - // If we got to the end of the file, we are done: - if (BIO_eof(in)) { - ERR_clear_error(); + // Alternatively, if we reached the end of the file and there was no error: + if (BIO_eof(in) && !ERR_peek_last_error()) { return certificates; } else { - // Otherwise we couldn't read everything so fail: + // Otherwise, we tried to read a certificate but failed somewhere: ossl_raise(eX509CertError, NULL); } } +static VALUE +load_chained_certificates_DER(BIO *in) { + X509 *x509 = d2i_X509_bio(in, NULL); + + // If we cannot read one certificate: + if (x509 == NULL) { + if (BIO_eof(in)) { + if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { + ERR_clear_error(); + } + } + + if (ERR_peek_last_error()) + ossl_raise(eX509CertError, NULL); + else + return Qnil; + } + + VALUE certificates = rb_ary_new(); + rb_ary_push(certificates, ossl_x509_new(x509)); + + return certificates; +} + +static VALUE +load_chained_certificates(VALUE _io) { + BIO *in = (BIO*)_io; + VALUE certificates = Qnil; + + // Case 1: certificate format is PEM: + certificates = load_chained_certificates_PEM(in); + + if (certificates != Qnil) + return certificates; + + // Case 2: certificate format is DER: + OSSL_BIO_reset(in); + + certificates = load_chained_certificates_DER(in); + + if (certificates != Qnil) + return certificates; + + // If we read to the end of the file, and no format matched: + if (BIO_eof(in)) { + return rb_ary_new(); + } + + // Otherwise we couldn't read the output correctly so fail: + ossl_raise(eX509CertError, "Could not detect format of certificate file!"); +} + static VALUE load_chained_certificates_ensure(VALUE _io) { BIO *in = (BIO*)_io; diff --git a/test/openssl/fixtures/pkey/empty.der b/test/openssl/fixtures/pkey/empty.der new file mode 100644 index 000000000..e69de29bb diff --git a/test/openssl/fixtures/pkey/fullchain.der b/test/openssl/fixtures/pkey/fullchain.der new file mode 100644 index 0000000000000000000000000000000000000000..7d44df8413aac2a7b71767f5926cb70c12bf77f2 GIT binary patch literal 1325 zcmXqLV%0QgVi8=x%*4pVB*YS}Y5&@@R;=l@RQB2j6>jsr40zc%wc0$|zVk9NaqoMbzx zr^>1MTJ>-D0P$2l@5%3;SHFF9AgW;h6U~I%=l@Fj-~4dnkJqkKJBxlrx;{HD*3gB7N;gn~D`9FALnz5aDA6~N0og0aB5EMqge86WfMgX| zTn(HJIM~?I8+jO+7!AzXxPiW9VQjh&^cw44LcK*FplvjT; zzOrX;*M9Rh(eEL$Grbg_w$5Nu;8m;I#dqM87_XMi(eSn)1)<0d% z`rr2QKG7YmwflVgdrMEX`EQpL>%S@fJLpwIxE*I>!r55CUu7cf=@OSWdFVyF71$Bv z9~?CKTXU>wg5=_tSGb&_8xFLn{9@i=sOc{3R1*BtXPNbD-E=+Ktrh=8I~6b6b^O{R z`AqletB4ubtz(~fH2u4JSH{`#LjH}XKF_lKLtowPc+26nJ-T}4`t$iS*NH6n;p8h9 z*>mvJPPx@S2laVA@YL Date: Thu, 13 May 2021 14:02:40 +1200 Subject: [PATCH 04/22] Requested changes. --- ext/openssl/ossl_x509cert.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 6c026edf1..e0e52f1be 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -708,11 +708,11 @@ static VALUE load_chained_certificates_PEM(BIO *in) { X509 *x509 = PEM_read_bio_X509(in, NULL, NULL, NULL); - // If we cannot read even one certificate: + /* If we cannot read even one certificate: */ if (x509 == NULL) { - // If we cannot read one certificate because we could not read the PEM encoding: + /* If we cannot read one certificate because we could not read the PEM encoding: */ if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { - ERR_clear_error(); + ossl_clear_error(); } if (ERR_peek_last_error()) @@ -729,19 +729,19 @@ load_chained_certificates_PEM(BIO *in) { X509_free(x509); } - // We tried to read one more certificate but could not read start line: + /* We tried to read one more certificate but could not read start line: */ if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { - // This is not an error, it means we are finished: - ERR_clear_error(); + /* This is not an error, it means we are finished: */ + ossl_clear_error(); return certificates; } - // Alternatively, if we reached the end of the file and there was no error: + /* Alternatively, if we reached the end of the file and there was no error: */ if (BIO_eof(in) && !ERR_peek_last_error()) { return certificates; } else { - // Otherwise, we tried to read a certificate but failed somewhere: + /* Otherwise, we tried to read a certificate but failed somewhere: */ ossl_raise(eX509CertError, NULL); } } @@ -750,11 +750,11 @@ static VALUE load_chained_certificates_DER(BIO *in) { X509 *x509 = d2i_X509_bio(in, NULL); - // If we cannot read one certificate: + /* If we cannot read one certificate: */ if (x509 == NULL) { if (BIO_eof(in)) { if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { - ERR_clear_error(); + ossl_clear_error(); } } @@ -775,13 +775,13 @@ load_chained_certificates(VALUE _io) { BIO *in = (BIO*)_io; VALUE certificates = Qnil; - // Case 1: certificate format is PEM: + /* Case 1: certificate format is PEM: */ certificates = load_chained_certificates_PEM(in); if (certificates != Qnil) return certificates; - // Case 2: certificate format is DER: + /* Case 2: certificate format is DER: */ OSSL_BIO_reset(in); certificates = load_chained_certificates_DER(in); @@ -789,12 +789,12 @@ load_chained_certificates(VALUE _io) { if (certificates != Qnil) return certificates; - // If we read to the end of the file, and no format matched: + /* If we read to the end of the file, and no format matched: */ if (BIO_eof(in)) { return rb_ary_new(); } - // Otherwise we couldn't read the output correctly so fail: + /* Otherwise we couldn't read the output correctly so fail: */ ossl_raise(eX509CertError, "Could not detect format of certificate file!"); } @@ -815,7 +815,7 @@ load_chained_certificates_ensure(VALUE _io) { static VALUE ossl_x509_load_chained_certificates(VALUE klass, VALUE path) { - ERR_clear_error(); + ossl_clear_error(); BIO *in = BIO_new(BIO_s_file()); From a52be6402de862457af6ca2285cc2187e4908386 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 14:28:52 +1200 Subject: [PATCH 05/22] Fix memory leak. --- ext/openssl/ossl_x509cert.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index e0e52f1be..de68ab438 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -723,6 +723,7 @@ load_chained_certificates_PEM(BIO *in) { VALUE certificates = rb_ary_new(); rb_ary_push(certificates, ossl_x509_new(x509)); + X509_free(x509); while ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL))) { rb_ary_push(certificates, ossl_x509_new(x509)); @@ -753,9 +754,9 @@ load_chained_certificates_DER(BIO *in) { /* If we cannot read one certificate: */ if (x509 == NULL) { if (BIO_eof(in)) { - if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { - ossl_clear_error(); - } + if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { + ossl_clear_error(); + } } if (ERR_peek_last_error()) @@ -766,6 +767,7 @@ load_chained_certificates_DER(BIO *in) { VALUE certificates = rb_ary_new(); rb_ary_push(certificates, ossl_x509_new(x509)); + X509_free(x509); return certificates; } @@ -801,6 +803,7 @@ load_chained_certificates(VALUE _io) { static VALUE load_chained_certificates_ensure(VALUE _io) { BIO *in = (BIO*)_io; + BIO_free(in); return Qnil; From 1724e5eb296630171250bbb6b5a9d1a916564327 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 14:40:32 +1200 Subject: [PATCH 06/22] Safely append certificate to certificates array. --- ext/openssl/ossl_x509cert.c | 52 +++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index de68ab438..102dca44b 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -704,12 +704,45 @@ ossl_x509_eq(VALUE self, VALUE other) return !X509_cmp(a, b) ? Qtrue : Qfalse; } +struct load_chained_certificates_arguments { + VALUE certificates; + X509 *certificate; +}; + +static VALUE +load_chained_certificates_append_push(VALUE _arguments) { + struct load_chained_certificates_arguments *arguments = (struct load_chained_certificates_arguments*)_arguments; + + rb_ary_push(arguments->certificates, ossl_x509_new(arguments->certificate)); + + return Qnil; +} + +static VALUE +load_chained_certificate_append_ensure(VALUE _arguments) { + struct load_chained_certificates_arguments *arguments = (struct load_chained_certificates_arguments*)_arguments; + + X509_free(arguments->certificate); + + return Qnil; +} + +inline static void +load_chained_certificates_append(VALUE certificates, X509 *certificate) { + struct load_chained_certificates_arguments arguments = { + .certificates = certificates, + .certificate = certificate + }; + + rb_ensure(load_chained_certificates_append_push, (VALUE)&arguments, load_chained_certificate_append_ensure, (VALUE)&arguments); +} + static VALUE load_chained_certificates_PEM(BIO *in) { - X509 *x509 = PEM_read_bio_X509(in, NULL, NULL, NULL); + X509 *certificate = PEM_read_bio_X509(in, NULL, NULL, NULL); /* If we cannot read even one certificate: */ - if (x509 == NULL) { + if (certificate == NULL) { /* If we cannot read one certificate because we could not read the PEM encoding: */ if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { ossl_clear_error(); @@ -722,12 +755,10 @@ load_chained_certificates_PEM(BIO *in) { } VALUE certificates = rb_ary_new(); - rb_ary_push(certificates, ossl_x509_new(x509)); - X509_free(x509); + load_chained_certificates_append(certificates, certificate); - while ((x509 = PEM_read_bio_X509(in, NULL, NULL, NULL))) { - rb_ary_push(certificates, ossl_x509_new(x509)); - X509_free(x509); + while ((certificate = PEM_read_bio_X509(in, NULL, NULL, NULL))) { + load_chained_certificates_append(certificates, certificate); } /* We tried to read one more certificate but could not read start line: */ @@ -749,10 +780,10 @@ load_chained_certificates_PEM(BIO *in) { static VALUE load_chained_certificates_DER(BIO *in) { - X509 *x509 = d2i_X509_bio(in, NULL); + X509 *certificate = d2i_X509_bio(in, NULL); /* If we cannot read one certificate: */ - if (x509 == NULL) { + if (certificate == NULL) { if (BIO_eof(in)) { if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { ossl_clear_error(); @@ -766,8 +797,7 @@ load_chained_certificates_DER(BIO *in) { } VALUE certificates = rb_ary_new(); - rb_ary_push(certificates, ossl_x509_new(x509)); - X509_free(x509); + load_chained_certificates_append(certificates, certificate); return certificates; } From 8965bbe3e9a682f8fc0ee51f649df2a79a4c6fa5 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 14:43:50 +1200 Subject: [PATCH 07/22] Create array within safety of `rb_ensure`. --- ext/openssl/ossl_x509cert.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 102dca44b..a605c45d8 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -713,6 +713,10 @@ static VALUE load_chained_certificates_append_push(VALUE _arguments) { struct load_chained_certificates_arguments *arguments = (struct load_chained_certificates_arguments*)_arguments; + if (arguments->certificates == Qnil) { + arguments->certificates = rb_ary_new(); + } + rb_ary_push(arguments->certificates, ossl_x509_new(arguments->certificate)); return Qnil; @@ -727,7 +731,7 @@ load_chained_certificate_append_ensure(VALUE _arguments) { return Qnil; } -inline static void +inline static VALUE load_chained_certificates_append(VALUE certificates, X509 *certificate) { struct load_chained_certificates_arguments arguments = { .certificates = certificates, @@ -735,6 +739,8 @@ load_chained_certificates_append(VALUE certificates, X509 *certificate) { }; rb_ensure(load_chained_certificates_append_push, (VALUE)&arguments, load_chained_certificate_append_ensure, (VALUE)&arguments); + + return arguments.certificates; } static VALUE @@ -754,8 +760,7 @@ load_chained_certificates_PEM(BIO *in) { return Qnil; } - VALUE certificates = rb_ary_new(); - load_chained_certificates_append(certificates, certificate); + VALUE certificates = load_chained_certificates_append(Qnil, certificate); while ((certificate = PEM_read_bio_X509(in, NULL, NULL, NULL))) { load_chained_certificates_append(certificates, certificate); @@ -796,10 +801,7 @@ load_chained_certificates_DER(BIO *in) { return Qnil; } - VALUE certificates = rb_ary_new(); - load_chained_certificates_append(certificates, certificate); - - return certificates; + return load_chained_certificates_append(Qnil, certificate); } static VALUE From 70c41cc0d0be10ba4b7c4ee2bbc1cbeb2880e06d Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 15:01:06 +1200 Subject: [PATCH 08/22] Validate certificate load order and subject. --- test/openssl/test_x509cert.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index c36b992b7..db992e506 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -286,6 +286,8 @@ def test_load_fullchain_pem fullchain_path = Fixtures.file_path("pkey", "fullchain.pem") certificates = OpenSSL::X509::Certificate.load(fullchain_path) assert_equal 2, certificates.size + assert_equal "/CN=www.codeotaku.com", certificates[0].subject.to_s + assert_equal "/C=US/O=Let's Encrypt/CN=R3", certificates[1].subject.to_s end def test_load_empty_der @@ -300,6 +302,7 @@ def test_load_fullchain_der # DER encoding can only contain one certificate: assert_equal 1, certificates.size + assert_equal "/CN=www.codeotaku.com", certificates[0].subject.to_s end def test_load_fullchain_garbage From 0c700f9de858a95783cb4973f8e0208385b4a63a Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 15:25:17 +1200 Subject: [PATCH 09/22] Changes as requested. --- ext/openssl/ossl_x509cert.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index a605c45d8..bbbca08a4 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -733,10 +733,9 @@ load_chained_certificate_append_ensure(VALUE _arguments) { inline static VALUE load_chained_certificates_append(VALUE certificates, X509 *certificate) { - struct load_chained_certificates_arguments arguments = { - .certificates = certificates, - .certificate = certificate - }; + struct load_chained_certificates_arguments arguments; + arguments.certificates = certificates; + arguments.certificate = certificate; rb_ensure(load_chained_certificates_append_push, (VALUE)&arguments, load_chained_certificate_append_ensure, (VALUE)&arguments); @@ -790,7 +789,8 @@ load_chained_certificates_DER(BIO *in) { /* If we cannot read one certificate: */ if (certificate == NULL) { if (BIO_eof(in)) { - if (ERR_GET_REASON(ERR_peek_last_error()) == ASN1_R_HEADER_TOO_LONG) { + /* If we didn't actually read anything: */ + if (BIO_tell(in) == 0) { ossl_clear_error(); } } @@ -850,8 +850,6 @@ load_chained_certificates_ensure(VALUE _io) { static VALUE ossl_x509_load_chained_certificates(VALUE klass, VALUE path) { - ossl_clear_error(); - BIO *in = BIO_new(BIO_s_file()); if (in == NULL) From f2b09a91a79b1f4d4203805f0febf284c0e2195f Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 18:16:06 +1200 Subject: [PATCH 10/22] Variable declaration must come first. --- ext/openssl/ossl_x509cert.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index bbbca08a4..c238a3ec8 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -744,6 +744,7 @@ load_chained_certificates_append(VALUE certificates, X509 *certificate) { static VALUE load_chained_certificates_PEM(BIO *in) { + VALUE certificates = Qnil; X509 *certificate = PEM_read_bio_X509(in, NULL, NULL, NULL); /* If we cannot read even one certificate: */ @@ -759,7 +760,7 @@ load_chained_certificates_PEM(BIO *in) { return Qnil; } - VALUE certificates = load_chained_certificates_append(Qnil, certificate); + certificates = load_chained_certificates_append(Qnil, certificate); while ((certificate = PEM_read_bio_X509(in, NULL, NULL, NULL))) { load_chained_certificates_append(certificates, certificate); From 2868d4f99a3430ac4435a85cefdb822da7c27d06 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 18:18:23 +1200 Subject: [PATCH 11/22] Extra documentation. --- ext/openssl/ossl_x509cert.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index c238a3ec8..1e0fc93b5 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -846,7 +846,19 @@ load_chained_certificates_ensure(VALUE _io) { * call-seq: * OpenSSL::X509::Certificate.load(path) -> [certs...] * - * Read the chained certificates from specified file path. + * Read the chained certificates from specified file path. Supports both PEM and + * DER encoded certificates. + * + * PEM is a text format and supports more than one certificate. + * + * DER is a binary format and only supports one certificate. + * + * If the file is empty, it will return nil/empty array/raise an exception. TBD. + * + * If the file contains corruption of any kind or the certificates cannot be + * processed for some reason, an exception of type + * +OpenSSL::X509::CertificateError+ will be raised, most likely with an obscure + * error message from deep within the implementation. Good luck with that. */ static VALUE ossl_x509_load_chained_certificates(VALUE klass, VALUE path) From 548a089228407ba8c28da75635d954080626d562 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 May 2021 18:24:39 +1200 Subject: [PATCH 12/22] Remove interface prototypes. --- lib/openssl/x509.rb | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/lib/openssl/x509.rb b/lib/openssl/x509.rb index 17bb940d4..6771b90c1 100644 --- a/lib/openssl/x509.rb +++ b/lib/openssl/x509.rb @@ -338,20 +338,6 @@ def pretty_print(q) q.text 'not_after='; q.pp self.not_after } end - - # TODO: sikuan: Load the certificates (PEM, DER, etc.) from file, using BIO - # input string (File path) - # output array (Array of Certificate objects) - def load_file(path) - return load_chained_cert_from_file(path) - end - - # TODO: sikuan: Load the certificates (PEM, DER, etc.) from BIO (various data sources) - # input cert content (DER(ASN1) or PEM) - # output array (Array of Certificate objects) - def read_certificates(cert_content) - raise NotImplementedError - end end class CRL From ede5f795e0a738fa39481c11db33899499932332 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 18:31:19 +1200 Subject: [PATCH 13/22] Rework interface to support loading directly from strings or IOs. --- ext/openssl/ossl_x509cert.c | 43 +++++++++++++++++++++++++---------- test/openssl/test_x509cert.rb | 20 ++++++++-------- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 1e0fc93b5..5982f68fb 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -844,10 +844,34 @@ load_chained_certificates_ensure(VALUE _io) { /* * call-seq: - * OpenSSL::X509::Certificate.load(path) -> [certs...] + * OpenSSL::X509::Certificate.load_file(path) -> [certs...] * - * Read the chained certificates from specified file path. Supports both PEM and - * DER encoded certificates. + * Read the chained certificates from specified file path. See + * +OpenSSL::X509::Certificate.load+ for more details. + */ +static VALUE +ossl_x509_load_file(VALUE klass, VALUE path) +{ + BIO *in = BIO_new(BIO_s_file()); + + if (in == NULL) + ossl_raise(eX509CertError, NULL); + + if (BIO_read_filename(in, StringValueCStr(path)) <= 0) { + BIO_free(in); + ossl_raise(eX509CertError, NULL); + } + + return rb_ensure(load_chained_certificates, (VALUE)in, load_chained_certificates_ensure, (VALUE)in); +} + +/* + * call-seq: + * OpenSSL::X509::Certificate.load(string) -> [certs...] + * OpenSSL::X509::Certificate.load(file) -> [certs...] + * + * Read the chained certificates from the given input. Supports both PEM + * and DER encoded certificates. * * PEM is a text format and supports more than one certificate. * @@ -861,22 +885,16 @@ load_chained_certificates_ensure(VALUE _io) { * error message from deep within the implementation. Good luck with that. */ static VALUE -ossl_x509_load_chained_certificates(VALUE klass, VALUE path) +ossl_x509_load(VALUE klass, VALUE buffer) { - BIO *in = BIO_new(BIO_s_file()); + BIO *in = ossl_obj2bio(&buffer); if (in == NULL) ossl_raise(eX509CertError, NULL); - if (BIO_read_filename(in, StringValueCStr(path)) <= 0) { - BIO_free(in); - ossl_raise(eX509CertError, NULL); - } - return rb_ensure(load_chained_certificates, (VALUE)in, load_chained_certificates_ensure, (VALUE)in); } - /* * INIT */ @@ -985,7 +1003,8 @@ Init_ossl_x509cert(void) */ cX509Cert = rb_define_class_under(mX509, "Certificate", rb_cObject); - rb_define_singleton_method(cX509Cert, "load", ossl_x509_load_chained_certificates, 1); + rb_define_singleton_method(cX509Cert, "load_file", ossl_x509_load_file, 1); + rb_define_singleton_method(cX509Cert, "load", ossl_x509_load, 1); rb_define_alloc_func(cX509Cert, ossl_x509_alloc); rb_define_method(cX509Cert, "initialize", ossl_x509_initialize, -1); diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index db992e506..cf11b481e 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -276,40 +276,40 @@ def test_marshal assert_equal cert.to_der, deserialized.to_der end - def test_load_empty_pem + def test_load_file_empty_pem empty_path = Fixtures.file_path("pkey", "empty.pem") - certificates = OpenSSL::X509::Certificate.load(empty_path) + certificates = OpenSSL::X509::Certificate.load_file(empty_path) assert_equal 0, certificates.size end - def test_load_fullchain_pem + def test_load_file_fullchain_pem fullchain_path = Fixtures.file_path("pkey", "fullchain.pem") - certificates = OpenSSL::X509::Certificate.load(fullchain_path) + certificates = OpenSSL::X509::Certificate.load_file(fullchain_path) assert_equal 2, certificates.size assert_equal "/CN=www.codeotaku.com", certificates[0].subject.to_s assert_equal "/C=US/O=Let's Encrypt/CN=R3", certificates[1].subject.to_s end - def test_load_empty_der + def test_load_file_empty_der empty_path = Fixtures.file_path("pkey", "empty.der") - certificates = OpenSSL::X509::Certificate.load(empty_path) + certificates = OpenSSL::X509::Certificate.load_file(empty_path) assert_equal 0, certificates.size end - def test_load_fullchain_der + def test_load_file_fullchain_der fullchain_path = Fixtures.file_path("pkey", "fullchain.der") - certificates = OpenSSL::X509::Certificate.load(fullchain_path) + certificates = OpenSSL::X509::Certificate.load_file(fullchain_path) # DER encoding can only contain one certificate: assert_equal 1, certificates.size assert_equal "/CN=www.codeotaku.com", certificates[0].subject.to_s end - def test_load_fullchain_garbage + def test_load_file_fullchain_garbage fullchain_path = Fixtures.file_path("pkey", "garbage.txt") assert_raise(OpenSSL::X509::CertificateError) do - certificates = OpenSSL::X509::Certificate.load(fullchain_path) + certificates = OpenSSL::X509::Certificate.load_file(fullchain_path) end end From b51241ff8a9ff3d5582ee4175857f1cd5a2917f9 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 18:49:36 +1200 Subject: [PATCH 14/22] Empty file/garbage file = exception. --- ext/openssl/ossl_x509cert.c | 7 +------ test/openssl/test_x509cert.rb | 11 +++-------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 5982f68fb..c57f95987 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -824,13 +824,8 @@ load_chained_certificates(VALUE _io) { if (certificates != Qnil) return certificates; - /* If we read to the end of the file, and no format matched: */ - if (BIO_eof(in)) { - return rb_ary_new(); - } - /* Otherwise we couldn't read the output correctly so fail: */ - ossl_raise(eX509CertError, "Could not detect format of certificate file!"); + ossl_raise(eX509CertError, "Could not detect format of certificate data!"); } static VALUE diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index cf11b481e..5f0f32efd 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -278,8 +278,9 @@ def test_marshal def test_load_file_empty_pem empty_path = Fixtures.file_path("pkey", "empty.pem") - certificates = OpenSSL::X509::Certificate.load_file(empty_path) - assert_equal 0, certificates.size + assert_raise(OpenSSL::X509::CertificateError) do + OpenSSL::X509::Certificate.load_file(empty_path) + end end def test_load_file_fullchain_pem @@ -290,12 +291,6 @@ def test_load_file_fullchain_pem assert_equal "/C=US/O=Let's Encrypt/CN=R3", certificates[1].subject.to_s end - def test_load_file_empty_der - empty_path = Fixtures.file_path("pkey", "empty.der") - certificates = OpenSSL::X509::Certificate.load_file(empty_path) - assert_equal 0, certificates.size - end - def test_load_file_fullchain_der fullchain_path = Fixtures.file_path("pkey", "fullchain.der") certificates = OpenSSL::X509::Certificate.load_file(fullchain_path) From 00f8c9191f9d640f94466fa1538f2753638e5550 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 19:35:00 +1200 Subject: [PATCH 15/22] Update documentation. --- ext/openssl/ossl_x509cert.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index c57f95987..880d21e3c 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -872,12 +872,8 @@ ossl_x509_load_file(VALUE klass, VALUE path) * * DER is a binary format and only supports one certificate. * - * If the file is empty, it will return nil/empty array/raise an exception. TBD. - * - * If the file contains corruption of any kind or the certificates cannot be - * processed for some reason, an exception of type - * +OpenSSL::X509::CertificateError+ will be raised, most likely with an obscure - * error message from deep within the implementation. Good luck with that. + * If the file is empty, or contains only unrelated data, an + * +OpenSSL::X509::CertificateError+ exception will be raised. */ static VALUE ossl_x509_load(VALUE klass, VALUE buffer) From 7ad565380ef4fe982772bfbe053364ac54728c93 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 21:25:15 +1200 Subject: [PATCH 16/22] Check for DER encoded certificate first. --- ext/openssl/ossl_x509cert.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 880d21e3c..3c7ca0faf 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -789,6 +789,11 @@ load_chained_certificates_DER(BIO *in) { /* If we cannot read one certificate: */ if (certificate == NULL) { + /* If we cannot read one certificate because we could not read the DER encoding: */ + if (ERR_GET_REASON(ERR_peek_last_error()) == ERR_R_NESTED_ASN1_ERROR) { + ossl_clear_error(); + } + if (BIO_eof(in)) { /* If we didn't actually read anything: */ if (BIO_tell(in) == 0) { @@ -810,16 +815,18 @@ load_chained_certificates(VALUE _io) { BIO *in = (BIO*)_io; VALUE certificates = Qnil; - /* Case 1: certificate format is PEM: */ - certificates = load_chained_certificates_PEM(in); + /* + DER is a binary format and it may contain octets within it that look like + PEM encoded certificates. So we need to check DER first. + */ + certificates = load_chained_certificates_DER(in); if (certificates != Qnil) return certificates; - /* Case 2: certificate format is DER: */ OSSL_BIO_reset(in); - certificates = load_chained_certificates_DER(in); + certificates = load_chained_certificates_PEM(in); if (certificates != Qnil) return certificates; From 61b4355b1c7471cb85e6a1bc341515549776917e Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 21:28:47 +1200 Subject: [PATCH 17/22] `ossl_obj2bio` does not return NULL. --- ext/openssl/ossl_bio.c | 20 ++++++++++++-------- ext/openssl/ossl_x509cert.c | 3 --- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/ext/openssl/ossl_bio.c b/ext/openssl/ossl_bio.c index 42833d901..1f01f5574 100644 --- a/ext/openssl/ossl_bio.c +++ b/ext/openssl/ossl_bio.c @@ -16,11 +16,14 @@ ossl_obj2bio(volatile VALUE *pobj) BIO *bio; if (RB_TYPE_P(obj, T_FILE)) - obj = rb_funcallv(obj, rb_intern("read"), 0, NULL); + obj = rb_funcallv(obj, rb_intern("read"), 0, NULL); + StringValue(obj); bio = BIO_new_mem_buf(RSTRING_PTR(obj), RSTRING_LENINT(obj)); + if (!bio) - ossl_raise(eOSSLError, "BIO_new_mem_buf"); + ossl_raise(eOSSLError, "BIO_new_mem_buf"); + *pobj = obj; return bio; } @@ -28,15 +31,16 @@ ossl_obj2bio(volatile VALUE *pobj) VALUE ossl_membio2str(BIO *bio) { - VALUE ret; + VALUE result; int state; - BUF_MEM *buf; + BUF_MEM *memory_buffer; - BIO_get_mem_ptr(bio, &buf); - ret = ossl_str_new(buf->data, buf->length, &state); + BIO_get_mem_ptr(bio, &memory_buffer); + result = ossl_str_new(memory_buffer->data, memory_buffer->length, &state); BIO_free(bio); + if (state) - rb_jump_tag(state); + rb_jump_tag(state); - return ret; + return result; } diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 3c7ca0faf..563fb0a93 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -887,9 +887,6 @@ ossl_x509_load(VALUE klass, VALUE buffer) { BIO *in = ossl_obj2bio(&buffer); - if (in == NULL) - ossl_raise(eX509CertError, NULL); - return rb_ensure(load_chained_certificates, (VALUE)in, load_chained_certificates_ensure, (VALUE)in); } From a6a9fe39af664f7df662db3fb2a9c64efc9bf5b3 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 21:31:51 +1200 Subject: [PATCH 18/22] Use `File.read` for `load_file`. --- ext/openssl/ossl_x509cert.c | 24 ------------------------ lib/openssl/x509.rb | 4 ++++ 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 563fb0a93..492b22470 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -844,29 +844,6 @@ load_chained_certificates_ensure(VALUE _io) { return Qnil; } -/* - * call-seq: - * OpenSSL::X509::Certificate.load_file(path) -> [certs...] - * - * Read the chained certificates from specified file path. See - * +OpenSSL::X509::Certificate.load+ for more details. - */ -static VALUE -ossl_x509_load_file(VALUE klass, VALUE path) -{ - BIO *in = BIO_new(BIO_s_file()); - - if (in == NULL) - ossl_raise(eX509CertError, NULL); - - if (BIO_read_filename(in, StringValueCStr(path)) <= 0) { - BIO_free(in); - ossl_raise(eX509CertError, NULL); - } - - return rb_ensure(load_chained_certificates, (VALUE)in, load_chained_certificates_ensure, (VALUE)in); -} - /* * call-seq: * OpenSSL::X509::Certificate.load(string) -> [certs...] @@ -998,7 +975,6 @@ Init_ossl_x509cert(void) */ cX509Cert = rb_define_class_under(mX509, "Certificate", rb_cObject); - rb_define_singleton_method(cX509Cert, "load_file", ossl_x509_load_file, 1); rb_define_singleton_method(cX509Cert, "load", ossl_x509_load, 1); rb_define_alloc_func(cX509Cert, ossl_x509_alloc); diff --git a/lib/openssl/x509.rb b/lib/openssl/x509.rb index 6771b90c1..8d5f7b240 100644 --- a/lib/openssl/x509.rb +++ b/lib/openssl/x509.rb @@ -338,6 +338,10 @@ def pretty_print(q) q.text 'not_after='; q.pp self.not_after } end + + def self.load_file(path) + load(File.read(path)) + end end class CRL From ff0bae186871a7fcd30d65114f6cf726beee19ad Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 22:55:42 +1200 Subject: [PATCH 19/22] Ensure binary encoding. --- lib/openssl/x509.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/openssl/x509.rb b/lib/openssl/x509.rb index 8d5f7b240..367a899e2 100644 --- a/lib/openssl/x509.rb +++ b/lib/openssl/x509.rb @@ -340,7 +340,7 @@ def pretty_print(q) end def self.load_file(path) - load(File.read(path)) + load(File.binread(path)) end end From 390964bae9e3b9e309091ba2ce52df9462db6381 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Wed, 19 May 2021 23:28:53 +1200 Subject: [PATCH 20/22] Ignore all errors if DER fails to load. --- ext/openssl/ossl_x509cert.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 492b22470..c4e9ff246 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -789,22 +789,10 @@ load_chained_certificates_DER(BIO *in) { /* If we cannot read one certificate: */ if (certificate == NULL) { - /* If we cannot read one certificate because we could not read the DER encoding: */ - if (ERR_GET_REASON(ERR_peek_last_error()) == ERR_R_NESTED_ASN1_ERROR) { - ossl_clear_error(); - } - - if (BIO_eof(in)) { - /* If we didn't actually read anything: */ - if (BIO_tell(in) == 0) { - ossl_clear_error(); - } - } + /* Ignore error. We could not load. */ + ossl_clear_error(); - if (ERR_peek_last_error()) - ossl_raise(eX509CertError, NULL); - else - return Qnil; + return Qnil; } return load_chained_certificates_append(Qnil, certificate); From f25b1c86ecb825a33c4d6d33b1cbc3c3aa272021 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 20 May 2021 09:00:46 +1200 Subject: [PATCH 21/22] Partially revert "`ossl_obj2bio` does not return NULL." This reverts commit 61b4355b1c7471cb85e6a1bc341515549776917e. --- ext/openssl/ossl_bio.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/ext/openssl/ossl_bio.c b/ext/openssl/ossl_bio.c index 1f01f5574..42833d901 100644 --- a/ext/openssl/ossl_bio.c +++ b/ext/openssl/ossl_bio.c @@ -16,14 +16,11 @@ ossl_obj2bio(volatile VALUE *pobj) BIO *bio; if (RB_TYPE_P(obj, T_FILE)) - obj = rb_funcallv(obj, rb_intern("read"), 0, NULL); - + obj = rb_funcallv(obj, rb_intern("read"), 0, NULL); StringValue(obj); bio = BIO_new_mem_buf(RSTRING_PTR(obj), RSTRING_LENINT(obj)); - if (!bio) - ossl_raise(eOSSLError, "BIO_new_mem_buf"); - + ossl_raise(eOSSLError, "BIO_new_mem_buf"); *pobj = obj; return bio; } @@ -31,16 +28,15 @@ ossl_obj2bio(volatile VALUE *pobj) VALUE ossl_membio2str(BIO *bio) { - VALUE result; + VALUE ret; int state; - BUF_MEM *memory_buffer; + BUF_MEM *buf; - BIO_get_mem_ptr(bio, &memory_buffer); - result = ossl_str_new(memory_buffer->data, memory_buffer->length, &state); + BIO_get_mem_ptr(bio, &buf); + ret = ossl_str_new(buf->data, buf->length, &state); BIO_free(bio); - if (state) - rb_jump_tag(state); + rb_jump_tag(state); - return result; + return ret; } From 511c2cba0f041ab3ea57b76841025824ae68ede6 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 20 May 2021 09:02:29 +1200 Subject: [PATCH 22/22] Update fixtures. --- .../pkey/{fullchain.der => certificate.der} | Bin test/openssl/fixtures/pkey/fullchain.pem | 2 +- test/openssl/fixtures/pkey/garbage.txt | 2 +- test/openssl/test_x509cert.rb | 4 ++-- 4 files changed, 4 insertions(+), 4 deletions(-) rename test/openssl/fixtures/pkey/{fullchain.der => certificate.der} (100%) diff --git a/test/openssl/fixtures/pkey/fullchain.der b/test/openssl/fixtures/pkey/certificate.der similarity index 100% rename from test/openssl/fixtures/pkey/fullchain.der rename to test/openssl/fixtures/pkey/certificate.der diff --git a/test/openssl/fixtures/pkey/fullchain.pem b/test/openssl/fixtures/pkey/fullchain.pem index 3eb6ef424..624785d32 100644 --- a/test/openssl/fixtures/pkey/fullchain.pem +++ b/test/openssl/fixtures/pkey/fullchain.pem @@ -53,4 +53,4 @@ S8MXjohyc9z9/G2948kLjmE6Flh9dDYrVYA9x2O+hEPGOaEOa1eePynBgPayvUfL qjBstzLhWVQLGAkXXmNs+5ZnPBxzDJOLxhF2JIbeQAcH5H0tZrUlo5ZYyOqA7s9p O5b85o3AM/OJ+CktFBQtfvBhcJVd9wvlwPsk+uyOy2HI7mNxKKgsBTt375teA2Tw UdHkhVNcsAKX1H7GNNLOEADksd86wuoXvg== ------END CERTIFICATE----- \ No newline at end of file +-----END CERTIFICATE----- diff --git a/test/openssl/fixtures/pkey/garbage.txt b/test/openssl/fixtures/pkey/garbage.txt index 5e1c309da..557db03de 100644 --- a/test/openssl/fixtures/pkey/garbage.txt +++ b/test/openssl/fixtures/pkey/garbage.txt @@ -1 +1 @@ -Hello World \ No newline at end of file +Hello World diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index 5f0f32efd..7e8e5e960 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -291,8 +291,8 @@ def test_load_file_fullchain_pem assert_equal "/C=US/O=Let's Encrypt/CN=R3", certificates[1].subject.to_s end - def test_load_file_fullchain_der - fullchain_path = Fixtures.file_path("pkey", "fullchain.der") + def test_load_file_certificate_der + fullchain_path = Fixtures.file_path("pkey", "certificate.der") certificates = OpenSSL::X509::Certificate.load_file(fullchain_path) # DER encoding can only contain one certificate: