Timestamp support #204

btoews · 2018-06-21T19:51:22Z

This PR attempts to revive Feature #4183 adding timestamp protocol support. That issue has three revisions of patches. I applied each as a commit on top of master. Unsurprisingly, the code didn't compile and tests didn't pass. I did the bare minimum to get tests passing in dce927c.

I'm not much of a C hacker and know very little about Ruby internals. I'll do my best to respond to any feedback on this PR, but I might need some help.

cc @tenderlove since he's been helping me on this already
cc @emboss since this is his patch
fixes #203

tenderlove · 2018-06-21T23:35:41Z

@mastahyeti it looks like OpenSSL 1.1.0 is getting errors because it changed a lot of these structures to be opaque. This means we have to use getters and setters to access the struct members. I'm not sure how to build against OpenSSL 1.1.0 yet, but we need to change the direct member access to getters / setters like this:

diff --git a/ext/openssl/ossl_ts.c b/ext/openssl/ossl_ts.c
index 9b9a6db..b8bb3b7 100755
--- a/ext/openssl/ossl_ts.c
+++ b/ext/openssl/ossl_ts.c
@@ -148,8 +148,8 @@ ossl_tsreq_alloc(VALUE klass)
         ossl_raise(eTimestampError, NULL);
         return Qnil;
     }
-    req->version = ASN1_INTEGER_new();
-    ASN1_INTEGER_set(req->version, 1);
+    TS_REQ_set_version(req, ASN1_INTEGER_new());
+    ASN1_INTEGER_set(TS_REQ_get_version(req), 1);
     req->extensions = NULL;
     if (!(req->msg_imprint = TS_MSG_IMPRINT_new())) {
         ossl_raise(eTimestampError, NULL);

tenderlove · 2018-06-21T23:36:50Z

OpenSSL change is openssl/openssl@ca4a494

rhenium · 2018-06-22T07:49:36Z

In 2016, I also tried to port the patch to the OpenSSL 1.1 API, but I didn't finish that. It does compile with OpenSSL 1.1.0, but the test does not pass.

https://github.com/rhenium/ruby-openssl/tree/ky/timestamp-support

tenderlove · 2018-06-22T18:54:07Z

@rhenium interesting. Do the tests pass on OpenSSL 1.0 using your patch?

btoews · 2018-07-06T14:28:16Z

All the OpenSSL builds are passing now. Almost there!

btoews · 2018-07-09T16:41:53Z

All the CI builds are passing now. I think this is ready for some 👀. Also, let me know what, if any, rebasing/squashing is needed.

btoews · 2018-07-16T17:28:59Z

Bump. I think this PR is ready for review.

btoews · 2018-07-23T18:39:24Z

@rhenium @zzak 👋 This PR revives Feature #4183 to add timestamp protocol support to this gem. Quite a few changes to that patch series were necessary because of changes to coding styles in this codebase as well as changes to OpenSSL. All the CI builds are passing, so I think this branch is ready for some review. Thanks.

rhenium

I'm sorry for the long delay in responding. I've added minor notes as inline comments.

As for the interface, the accessors for the fields of TSTInfo are defined on the Timestamp::Response, but they should be moved to their own class (say, Timestamp::TokenInfo) since they aren't direct properties of a response but of a token that the response merely carries.

rhenium · 2018-07-27T08:15:31Z

ext/openssl/ossl_ts.c

+VALUE eTimestampError, eCertValidationError;
+VALUE cTimestampRequest;
+VALUE cTimestampResponse;
+VALUE cTimestampFactory;


These variables can be static.

rhenium · 2018-07-27T08:16:21Z

ext/openssl/ossl_ts.c

+}
+
+TS_REQ *
+GetTsReqPtr(VALUE obj)


Since we now use TypedData, the callers can use GetTSRequest() directly.

ext/openssl/ossl_ts.c

rhenium · 2018-07-27T08:20:23Z

ext/openssl/ossl_ts.c

+ obj = obj_to_asn1obj(algo);
+ mi = TS_REQ_get_msg_imprint(req);
+ algor = TS_MSG_IMPRINT_get_algo(mi);
+ if (!X509_ALGOR_set0(algor, obj, V_ASN1_NULL, NULL))


obj must be free'd in the error path.

rhenium · 2018-07-27T08:22:30Z

ext/openssl/ossl_ts.c

+
+ GetTSRequest(self, req);
+ mi = TS_REQ_get_msg_imprint(req);
+ if (!TS_MSG_IMPRINT_set_msg(mi, (unsigned char *)RSTRING_PTR(hash), RSTRING_LEN(hash)))


RSTRING_LEN -> RSTRING_LENINT

rhenium · 2018-08-06T13:02:23Z

ext/openssl/ossl_ts.c

+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_sha224)));
+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_sha256)));
+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_sha384)));
+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_sha512)));


EVP_get_digestbyname(OBJ_nid2sn(NID_md5)) -> EVP_md5()

rhenium · 2018-08-06T13:13:25Z

test/test_ts.rb

+ ts = fac.create_timestamp(ee_key, intermediate_cert, req)
+
+ ts.verify(req, [ca_cert])
+ end


TimestampError is actually raised by #create_timestamp, not by #verify.

rhenium · 2018-08-06T15:45:57Z

ext/openssl/ossl_ts.c

+ if (ERR_GET_LIB(e) == ERR_LIB_TS) {
+ if (ERR_GET_REASON(e) == TS_R_CERTIFICATE_VERIFY_ERROR)
+ is_validation_err = 1;
+ }


The use of ERR_GET_{LIB,REASON}() makes me nervous because the error codes are not explicitly stated as part of the public API.

Is the distinction in the raised exception actually important? I don't know. But otherwise, I'd suggest doing simply ossl_raise(eTimestampError, "TS_RESP_verify_response").

I think the caller might care whether there was an issue building the certificate chain versus the signature itself being invalid. If an exception is raised though, the caller can use other APIs to figure out if the issue was with the certificate chain. I'm fine with always raising TimestampError.

rhenium · 2018-08-06T16:49:19Z

ext/openssl/ossl_ts.c

+ * OpenSSL::PKCS7.
+ *
+ * call-seq:
+ * response.pkcs7 -> nil or OpenSSL::PKCS7


Maybe just name the method token? It sounds better to me.

rhenium · 2018-08-06T17:00:41Z

ext/openssl/ossl_ts.c

+ si = TS_RESP_get_status_info(resp);
+ text = TS_STATUS_INFO_get0_text(si);
+ if (!text)
+ return Qnil;


It might be better to return an empty array in this case.

I haven't seen any timestamp servers return more than one string. It might be a nicer API if we just returned the strings joined together or nil. What do you think?

btoews · 2018-08-08T15:26:34Z

ext/openssl/ossl_ts.c

+ TS_RESP_CTX_set_time_cb(ctx, ossl_tsfac_time_cb, &gen_time);
+
+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_md5)));
+ TS_RESP_CTX_add_md(ctx, EVP_get_digestbyname(OBJ_nid2sn(NID_sha1)));


Would it be okay to take an opinionated stance and not support SHA1 and MD5?

I started wondering where the list comes in the first place. I suspect the best thing would be to not have a hard-coded list and to let the user set their own one.

👍 I'll refactor this to allow the user to specific a list.

I added a Factory#allowed_digests accessor in 5e45f13. Factory#create_timestamp now requires the list of allowed digest algorithms to be provided.

btoews · 2018-08-09T15:58:52Z

@rhenium Thanks for the thorough review! I think I have addressed all your feedback in fb90bea...703e821.

rhenium · 2018-08-16T08:56:11Z

ext/openssl/ossl_ts.c

+}
+
+void
+ossl_ts_verify_ctx_free(TS_VERIFY_CTX *ctx)


This function is never used.

rhenium · 2018-08-16T08:58:24Z

ext/openssl/ossl_ts.c

+ SetTSResponse(ret, response);
+
+end:
+ if(asn1_serial) ASN1_INTEGER_free(asn1_serial);


*_free(NULL) are no-op, so the if conditions can be removed.

rhenium · 2018-08-16T09:00:21Z

ext/openssl/ossl_ts.c

+ }
+ if (status)
+ rb_jump_tag(status);
+ return ret;


Though it's a false-positive, GCC says ‘ret’ may be used uninitialized in this function.

rhenium · 2018-08-16T09:01:03Z

ext/openssl/ossl_ts.c

+ ASN1_OBJECT *def_policy_id_obj = NULL;
+ long lgen_time;
+ const char * err_msg = NULL;
+ int i, status = 0;


i and cert are never used.

ext/openssl/ossl_ts.c

rhenium · 2018-08-16T09:11:13Z

ext/openssl/ossl_ts.c

+ TS_RESP_CTX_add_md(ctx, EVP_sha512());
+
+ str = rb_funcall(request, rb_intern("to_der"), 0);
+ req_bio = ossl_obj2bio(&str);


rb_funcall() and ossl_obj2bio() can raise an exception. By the way, the rb_funcall() can be replaced by ossl_to_der(request) (this still raises an exception).

rhenium · 2018-08-16T09:11:35Z

ext/openssl/ossl_ts.c

+ goto end;
+ }
+
+ ret = NewTSResponse(cTimestampResponse);


NewTSResponse() also raises an exception.

rhenium · 2018-08-16T09:20:33Z

ext/openssl/ossl_ts.c

+ for (i=0; i < sk_X509_num(p7->d.sign->cert); i++) {
+ cert = sk_X509_value(p7->d.sign->cert, i);
+ X509_up_ref(cert);
+ sk_X509_push(x509inter, cert);


Check the return value.

rhenium · 2018-08-16T09:21:50Z

ext/openssl/ossl_ts.c

+ rb_jump_tag(status);
+ }
+ } else {
+ x509inter = sk_X509_new_null();


The function can fail.

rhenium · 2018-08-16T09:26:04Z

ext/openssl/ossl_ts.c

+ lgen_time = NUM2LONG(rb_funcall(gen_time, rb_intern("to_i"), 0));
+
+ serial_number = ossl_tsfac_get_serial_number(self);
+ if (serial_number == Qnil) {


Comparison with nil should be done by NIL_P(val). This is mostly a style thing.

rhenium · 2018-08-16T09:35:24Z

For some reason, the newly added files have an executable bit. Please remove them.

And there seem some unused variables in the test code.

/home/k/work/ruby-openssl/test/utils.rb:328: warning: assigned but unused variable - now
/home/k/work/ruby-openssl/test/utils.rb:341: warning: assigned but unused variable - now
/home/k/work/ruby-openssl/test/utils.rb:356: warning: assigned but unused variable - now
/home/k/work/ruby-openssl/test/utils.rb:370: warning: assigned but unused variable - now
/home/k/work/ruby-openssl/test/test_ts.rb:334: warning: assigned but unused variable - req

btoews · 2018-08-21T00:28:14Z

I think this is ready for another round of review 🙇

btoews · 2018-08-21T00:33:02Z

ext/openssl/ossl_ts.c

+ ASN1_INTEGER **snptr = (ASN1_INTEGER **)data;
+ ASN1_INTEGER *sn = *snptr;
+ *snptr = NULL;
+ return sn;


Some context here: This callback is given the serial number by ossl_tsfac_create_ts. OpenSSL calls the callback to get the serial number and then eventually frees it for us. If OpenSSL hits an error before it calls the callback though, ossl_tsfac_create_ts has no way of knowing that the serial number didn't get freed. So, I'm passing the serial number in as a ASN1_INTEGER ** so we can avoid leaking or double-freeing in ossl_tsfac_create_ts.

Maybe you can add this as a comment to the code.

btoews · 2018-09-05T22:26:20Z

Just a gentle reminder: this branch is ready for another round of review.

btoews · 2018-10-03T16:16:02Z

Bump 😬

vcsjones · 2018-11-28T15:59:19Z

Hi! This functionality would be very useful to me. Friendly +1 from an interested 3rd party 😊.

clairernovotny · 2019-02-21T15:55:06Z

Another interested 3rd party. Support for this would be helpful.

ioquatix · 2019-06-15T14:05:58Z

Can you rebase on master and I will review.

btoews · 2019-06-19T16:34:44Z

Can you rebase on master and I will review.

Done 😄

btoews · 2019-08-08T14:44:17Z

@ioquatix Just checking in. This should be ready to merge.

This commit applies the initial patches (ts.tar.gz) from https://bugs.ruby-lang.org/issues/4183 This compiles with several warnings. Tests don't run yet.

This commit applies the second patches (ts2.tar.gz) from https://bugs.ruby-lang.org/issues/4183

This commit applies the third patches (tsr3.tar.gz) from https://bugs.ruby-lang.org/issues/4183

A number of conventions seem to have changed, causing a fair bit of breakage: - `Data_*` was deprecated in favor of `TypedData_*` - `ossl_obj2bio` takes a `VALUE*` instead of `VALUE` now - `time_to_time_t()` was removed

- clean up whitespace - be consistent with not returning after ossl_raise - use accessor functions when working with openssl TS_* structs - backport accessors for TS_STATUS_INFO, TS_VERIFY_CTX, and TS_RESP_CTX as macros

- define missing TS_RESP_CTX_set_time_cb - handle alternate case for nil oid

- make some global variables static instead of extern - get rid of GetTsReqPtr/GetTsRespPtr functions - don't use c99 comments - fix some leaks - clarify what numeric type is returned (Integer or BN, never Fixnum) - typos - add missing checks, remove unecessary checks - use OPENSSL_NO_TS instead of our own macros checking for ts support - use EVP_{digest-name} instead of looking up algos by NID - don't differentiate between failure reasons when verifying - rename Response#pkcs7 to #token

This method allowed roots and intermediates to be specified in a number of ways. This complexity wasn't super valuable though and its better to only allow an X509::Store with an optional Array of intermediates. This greatly simplifies the code and fixes a few leaks.

btoews · 2019-09-27T15:08:45Z

@ioquatix Any more thoughts on this one? I just rebased onto master again.

ioquatix · 2019-09-27T21:18:16Z

Because this is new code can you please expand all tabs? Mixing tabs and spaces in C code in MRI is now old school.

ext/openssl/ossl_ts.c

ioquatix · 2019-09-27T21:25:32Z

If the formatting issues can be resolved I will merge this, it looks great.

btoews · 2019-09-30T19:42:53Z

Awesome. I've always hated that C style 😄 It would be nice to do a global search/replace across this repo for consistency.

ioquatix · 2019-09-30T22:25:32Z

Thanks for your persistence with this PR.

btoews · 2019-10-01T12:54:21Z

Thanks for merging it! I'm happy to see this finally land

israelb · 2020-02-14T21:53:02Z

@mastahyeti How can I use this new feature? I need the timestamps but it was impossible, I got the next error:

NameError: uninitialized constant OpenSSL::Timestamp

and the gem is pointing to master

gem 'openssl', git: 'https://github.com/ruby/openssl'

rhenium requested changes Aug 6, 2018

View reviewed changes

btoews commented Aug 8, 2018

View reviewed changes

rhenium requested changes Aug 16, 2018

View reviewed changes

btoews commented Aug 21, 2018

View reviewed changes

btoews mentioned this pull request Feb 21, 2019

How to specify a timestamp server github/smimesign#47

Open

emboss and others added 7 commits September 27, 2019 09:08

ts: Timestamp support (1/3)

c984360

This commit applies the initial patches (ts.tar.gz) from https://bugs.ruby-lang.org/issues/4183 This compiles with several warnings. Tests don't run yet.

ts: Timestamp support (2/3)

285bd40

This commit applies the second patches (ts2.tar.gz) from https://bugs.ruby-lang.org/issues/4183

ts: Timestamp support (3/3)

16b80b2

This commit applies the third patches (tsr3.tar.gz) from https://bugs.ruby-lang.org/issues/4183

ts: get tests running/passing

f15cd66

A number of conventions seem to have changed, causing a fair bit of breakage: - `Data_*` was deprecated in favor of `TypedData_*` - `ossl_obj2bio` takes a `VALUE*` instead of `VALUE` now - `time_to_time_t()` was removed

ts: update to work with openssl 1.1.x

96501f2

- clean up whitespace - be consistent with not returning after ossl_raise - use accessor functions when working with openssl TS_* structs - backport accessors for TS_STATUS_INFO, TS_VERIFY_CTX, and TS_RESP_CTX as macros

ts: use newer TypedData_* macros instead of Data_* ones

e03e6bf

ts: clean up some memory leaks

221241c

btoews added 10 commits September 27, 2019 09:08

ts: fix for libressl

ed44b34

- define missing TS_RESP_CTX_set_time_cb - handle alternate case for nil oid

ts: fix windows build

c7e2514

ts: simplify signature for TimestampFactory#additional_certs also

154c5f5

ts: don't do anything that might raise in callbacks

8a705bf

ts: fix another leak

da9cc7e

ts: move TS_TST_INFO accessors to a new Timestamp::TokenInfo class

f9be45d

ts: address more feedback

f2824e0

ts: require list of allowed digest algos for Factory

e832b8c

ioquatix reviewed Sep 27, 2019

View reviewed changes

ext/openssl/ossl_ts.c Outdated Show resolved Hide resolved

expand tabs

75700f1

ioquatix merged commit 775a8c0 into ruby:master Sep 30, 2019

btoews deleted the ts branch October 1, 2019 12:54

Timestamp support #204

Timestamp support #204

Conversation

btoews commented Jun 21, 2018

tenderlove commented Jun 21, 2018

tenderlove commented Jun 21, 2018

rhenium commented Jun 22, 2018

tenderlove commented Jun 22, 2018

btoews commented Jul 6, 2018

btoews commented Jul 9, 2018

btoews commented Jul 16, 2018

btoews commented Jul 23, 2018

rhenium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btoews commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium commented Aug 16, 2018

btoews commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btoews commented Sep 5, 2018

btoews commented Oct 3, 2018

vcsjones commented Nov 28, 2018

clairernovotny commented Feb 21, 2019

ioquatix commented Jun 15, 2019

btoews commented Jun 19, 2019

btoews commented Aug 8, 2019

btoews commented Sep 27, 2019

ioquatix commented Sep 27, 2019

ioquatix commented Sep 27, 2019

btoews commented Sep 30, 2019

ioquatix commented Sep 30, 2019

btoews commented Oct 1, 2019

israelb commented Feb 14, 2020