Feature #4183

[ext/openssl] Timestamp support

Added by Martin Bosslet over 3 years ago. Updated over 1 year ago.

[ruby-core:33801]
Status:Assigned
Priority:Normal
Assignee:Martin Bosslet
Category:ext
Target version:next minor

Description

=begin
Hi,

I'd like to propose the attached code to support timestamp creation and verification in ext/openssl under a new module OpenSSL::Timestamp. It comes also with various tests and rDoc documentation. Since OpenSSL only supports timestamps beginning with OpenSSL 1.0, the entire code is in #if brackets and the module is defined only for OpenSSL versions >= 1.0.

Best regards,
Martin
=end

ts.tar.gz (15.1 KB) Martin Bosslet, 12/22/2010 03:15 AM

ts2.tar.gz (15.3 KB) Martin Bosslet, 12/27/2010 01:07 AM

ts3.tar.gz (14.9 KB) Martin Bosslet, 12/29/2010 11:18 PM

History

#1 Updated by Aaron Patterson over 3 years ago

  • Assignee set to Aaron Patterson
  • Target version changed from 1.9.2 to 2.0.0

=begin

=end

#2 Updated by Aaron Patterson over 3 years ago

=begin
On Wed, Dec 22, 2010 at 03:19:12AM +0900, Martin Bosslet wrote:

Feature #4183: [ext/openssl] Timestamp support
http://redmine.ruby-lang.org/issues/show/4183

Author: Martin Bosslet
Status: Open, Priority: Normal
Category: ext, Target version: 1.9.2

Hi,

I'd like to propose the attached code to support timestamp creation and verification in ext/openssl under a new module OpenSSL::Timestamp. It comes also with various tests and rDoc documentation. Since OpenSSL only supports timestamps beginning with OpenSSL 1.0, the entire code is in #if brackets and the module is defined only for OpenSSL versions >= 1.0.

I think this is good functionality to add.

The patch seems good. I noticed some code that is not necessary though.
You can remove these lines:

 /*
  * Creates a Factory.
  *
  * call-seq:
  *       OpenSSL::Timestamp::Factory.new    -> Factory
  */
 static VALUE
 ossl_tsfac_initialize(VALUE self)
 {
     return self;
 }

 rb_define_method(cTimestampFactory, "initialize", ossl_tsfac_initialize, 0);

and everything will Just Work™.

This patch is really long, so if nobody objects after a week or two,
I'll apply.

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

#3 Updated by Nobuyoshi Nakada over 3 years ago

=begin
Hi,

At Sat, 25 Dec 2010 06:32:14 +0900,
Aaron Patterson wrote in :

The patch seems good. I noticed some code that is not necessary though.

Just curious for my eyes,
In ossltsverify():
1068 if (!(ctx->store = X509STOREnew())) {
1069 ossl_raise(eTimestampError, NULL);
1070 goto end;
1071 }

Jump to clean-up after raise?

1109 end:
1110 TSVERIFYCTX_free(ctx);
1111 return ret;

In ossltsfaccreatets():
1231 if (!(inter
certs = skX509new_null())) goto end;

When inter_certs can't get created, all the rest are just skipped?

1232 if (tsacert)
1233 if (rb
objiskindof(additionalcerts, rbcArray)) {
1234 for (i = 0; i < RARRAY
LEN(additionalcerts); i++) {
1235 cert = rb
aryentry(additionalcerts, i);
1236 skX509push(intercerts, GetX509CertPtr(cert));
1237 }
1238 }
1239 else {
1240 sk
X509push(intercerts, GetX509CertPtr(additional_certs));
1241 }

Just indentation of 1233..1241 is wrong, or 1232 is misplaced?

A patch to suppress some warnings.

diff --git i/ext/.document w/ext/.document
index 6f4f668..3c5ff49 100644
--- i/ext/.document
+++ w/ext/.document
@@ -37,6 +37,7 @@ openssl/osslpkeyrsa.c
openssl/osslrand.c
openssl/ossl
ssl.c
openssl/osslsslsession.c
+openssl/osslts.c
openssl/ossl
x509.c
openssl/osslx509attr.c
openssl/ossl
x509cert.c
diff --git i/ext/openssl/extconf.rb w/ext/openssl/extconf.rb
index b1f2d88..14e2745 100644
--- i/ext/openssl/extconf.rb
+++ w/ext/openssl/extconf.rb
@@ -127,6 +127,7 @@ end
havestructmember("EVPCIPHERCTX", "flags", "openssl/evp.h")
havestructmember("EVPCIPHERCTX", "engine", "openssl/evp.h")
havestructmember("X509ATTRIBUTE", "single", "openssl/x509.h")
+have
header("openssl/ts.h")

message "=== Checking done. ===\n"

diff --git i/ext/openssl/ossl.c w/ext/openssl/ossl.c
index 1e4f935..9a8d02e 100644
--- i/ext/openssl/ossl.c
+++ w/ext/openssl/ossl.c
@@ -865,6 +865,9 @@ Initopenssl()
Init
osslpkey();
Init
osslrand();
Init
osslssl();
+#if HAVE
OPENSSLTSH
+ Initosslts();
+#endif
Initosslx509();
Initosslocsp();
Initosslengine();
diff --git i/ext/openssl/ossl.h w/ext/openssl/ossl.h
index 4bb18d5..6b2ddae 100644
--- i/ext/openssl/ossl.h
+++ w/ext/openssl/ossl.h
@@ -59,6 +59,9 @@ extern "C" {
#include
#include
#include
+#if HAVEOPENSSLTSH
+# include
+#endif
#undef X509
NAME
#undef PKCS7SIGNERINFO
#if defined(HAVEOPENSSLENGINEH) && defined(HAVESTENGINE)
@@ -215,6 +218,9 @@ void ossl
debug(const char *, ...);
#include "osslpkey.h"
#include "ossl
rand.h"
#include "osslssl.h"
+#if HAVE
OPENSSLTSH
+# include "osslts.h"
+#endif
#include "ossl
version.h"
#include "osslx509.h"
#include "ossl
engine.h"
diff --git i/ext/openssl/osslts.c w/ext/openssl/osslts.c
old mode 100755
new mode 100644
index a0b8ca2..6adc8d9
--- i/ext/openssl/osslts.c
+++ w/ext/openssl/ossl
ts.c
@@ -110,19 +110,6 @@ objtoasn1obj(VALUE obj)
return a1obj;
}

-static ASN1STRING*
-obj
toasn1str(VALUE obj)
-{
- ASN1
STRING *str;
-
- StringValue(obj);
- if(!(str = ASN1STRINGnew()))
- osslraise(eASN1Error, NULL);
- ASN1
STRINGset(str, RSTRINGPTR(obj), RSTRINGLEN(obj));
-
- return str;
-}
-
static VALUE
get
asn1obj(ASN1OBJECT *obj)
{
@@ -319,7 +306,7 @@ ossl
tsreqsetmsgimprint(VALUE self, VALUE hash)
ossl
raise(eTimestampError, NULL);
return self;
}
- TSMSGIMPRINTsetmsg(mi, RSTRINGPTR(hash), RSTRINGLEN(hash));
+ TSMSGIMPRINTsetmsg(mi, (unsigned char *)RSTRINGPTR(hash), RSTRINGLEN(hash));

  return hash;

}
@@ -401,6 +388,7 @@ ossltsreqsetpolicyid(VALUE self, VALUE oid)
ASN1OBJECTfree(req->policyid);
obj = obj
toasn1obj(oid);
req->policy
id = obj;
+ return oid;
}

/*
@@ -536,7 +524,8 @@ ossltsrespalloc(VALUE klass)
* OpenSSL::Timestamp::Response.new(string) -> response
*/
static VALUE
-ossltsinitialize(VALUE self, VALUE der) {
+ossltsinitialize(VALUE self, VALUE der)
+{
TSRESP *tsresp = DATA_PTR(self);
BIO *in;

@@ -666,8 +655,6 @@ ossltsgetpkcs7(VALUE self)
{
TS
RESP *resp;
PKCS7 *p7;
- unsigned char *p;
- int len;

  GetTS_RESP(self, resp);
  p7 = resp->token;

@@ -920,7 +907,7 @@ ossltsto_der(VALUE self)
}

static void
-intosslhandleverifyerrors()
+intosslhandleverifyerrors(void)
{
const char *msg = NULL;
int isvalidationerr = 0;
@@ -1124,7 +1111,8 @@ ossltsfacinitialize(VALUE self)
}

static ASN1INTEGER *
-ossl
tsfacserialcb(struct TSrespctx ctx, void *data) {
+ossltsfacserialcb(struct TSrespctx *ctx, void *data)
+{
VALUE serial = *((VALUE *)data);
ASN1
INTEGER *num;
if (!(num = ASN1INTEGERnew())) {
@@ -1280,7 +1268,7 @@ end:
* INIT
*/
void
-Initosslts()
+Initosslts(void)
{
#if 0
mOSSL = rbdefinemodule("OpenSSL"); /
let rdoc know about mOSSL */
@@ -1543,4 +1531,4 @@ Initosslts()
rbdefinemethod(cTimestampFactory, "createtimestamp", ossltsfaccreatets, 3);
}

-#endif
\ No newline at end of file
+#endif
diff --git i/ext/openssl/osslts.h w/ext/openssl/osslts.h
index bbf3a0a..5034443 100755
--- i/ext/openssl/osslts.h
+++ w/ext/openssl/ossl
ts.h
@@ -22,4 +22,4 @@ extern VALUE cTimestampFactory;
void Initosslts(void);
TS_RESP *GetTsRespPtr(VALUE obj);

-#endif
\ No newline at end of file
+#endif

--
Nobu Nakada

=end

#4 Updated by Hiroshi Nakamura over 3 years ago

=begin
Hi,

On Sat, Dec 25, 2010 at 06:32, Aaron Patterson
aaron@tenderlovemaking.com wrote:

This patch is really long, so if nobody objects after a week or two,
I'll apply.

Isn't it good to ask him to be a ruby-ossl maintainer, instead of
applying the patch we don't understand?

Regards,
// NaHi

=end

#5 Updated by Aaron Patterson over 3 years ago

=begin
On Sat, Dec 25, 2010 at 11:58:56PM +0900, Hiroshi Nakamura wrote:

Hi,

On Sat, Dec 25, 2010 at 06:32, Aaron Patterson
aaron@tenderlovemaking.com wrote:

This patch is really long, so if nobody objects after a week or two,
I'll apply.

Isn't it good to ask him to be a ruby-ossl maintainer, instead of
applying the patch we don't understand?

I don't mind spending time to maintain openssl, but a patch this long is
difficult for just one person to review. I take responsibility for
patches I apply, so if there are bugs, I will fix them.

That being said, we can always use help! :-D

--
Aaron Patterson
http://tenderlovemaking.com/

Attachment: (unnamed)
=end

#6 Updated by Martin Bosslet over 3 years ago

=begin
Thanks for the input! I updated the code with regard to Aaron's and Nobuyoshi's comments. Concerning Nobuyoshi's questions:

Just curious for my eyes,
In ossltsverify():
1068 if (!(ctx->store = X509STOREnew())) {
1069 ossl_raise(eTimestampError, NULL);
1070 goto end;
1071 }

Jump to clean-up after raise?

1109 end:
1110 TSVERIFYCTX_free(ctx);
1111 return ret;

I corrected this, cleanup is now performed before the raise.

In ossltsfaccreatets():
1231 if (!(inter
certs = skX509new_null())) goto end;

When inter_certs can't get created, all the rest are just skipped?

I forgot to set the error message there. Corrected this, now an error is raised when allocation fails.

1232 if (tsacert)
1233 if (rb
objiskindof(additionalcerts, rbcArray)) {
1234 for (i = 0; i < RARRAY
LEN(additionalcerts); i++) {
1235 cert = rb
aryentry(additionalcerts, i);
1236 skX509push(intercerts, GetX509CertPtr(cert));
1237 }
1238 }
1239 else {
1240 sk
X509push(intercerts, GetX509CertPtr(additional_certs));
1241 }

Just indentation of 1233..1241 is wrong, or 1232 is misplaced?

No, you're right, it was simply misplaced, I removed it.

Regarding your patch, I already applied it in the update.

There is still some functionality that is not supported and some features I dislike about the way OpenSSL handles timestamp verification. Concerning the missing functionality - I'm planning to add this in case the feature will be accepted, I first wanted to get your reactions before I indulge too deeply in something nobody would want anyway :) Missing features right now are:

  • Extensions are neither supported for Request nor for the Response
  • Accuracy and TSAName are not supported for the Response.
  • I'd like to add some factory methods, that create Responses indicating errors easily. E.g.

    errorts = OpenSSL::Timestamp::Response.createfaultresponse(OpenSSL::Timestamp::Response::REJECTION,
    :UNACCEPTED
    POLICY,
    "The policy you provided is not supported by this server")

    This would simplify error creation for errors detected on the server side when manually analyzing the Request.

    Things I would like different:

  • I don't like the fact that for verification, you need to pass OpenSSL a BIO, even if you're still in possession of the TSREQ*, as in our case (and I guess in most use cases). This forces encoding (and later decoding in OpenSSL) the TSREQ*, which is unnecessary. I'll try to submit a patch to OpenSSL regarding this.

  • The second thing I'm not really happy with is the fact that you cannot validate a timestamp solely based on the timestamp end entity certificate, verification always includes certificate validation of the entire chain. This is too tightly coupled in my opinion, certificate validation should only be an option (so that there is the possibility to perform it in a separate step). In addition, decoupling would also remove the necessity to provide intermediate certificates or root certificates. If the timestamp already contains the timestamp authority certificate(which it must if Request#cert_requested? is true), validation would be self-contained, no other external resources needed. I'll also try to submit a patch for this to OpenSSL.

    @Hiroshi: thanks for the confidence, I'm happy to help :)

    Regards,
    Martin

=end

#7 Updated by Martin Bosslet over 3 years ago

=begin

  • I don't like the fact that for verification, you need to pass OpenSSL a BIO, even if you're still in possession of the TSREQ*, as in >our case (and I guess in most use cases). This forces encoding (and later decoding in OpenSSL) the TSREQ*, which is unnecessary. I'll >try to submit a patch to OpenSSL regarding this.

I meant for creation of a TS_RESP, not verification.

=end

#8 Updated by Martin Bosslet over 3 years ago

=begin
- Removed a redundant variable
- Corrected another code line with "raise without freeing before"
- Additional test
- replaced FileUtils by File method in the test

The attachment contains the two relevant files only, together with diff files showing the changes made to the versions in ts2.tar.gz (since changes are really minor).

Regards,
Martin
=end

#9 Updated by Shyouhei Urabe about 3 years ago

  • Status changed from Open to Assigned

#10 Updated by Martin Bosslet almost 3 years ago

  • Assignee changed from Aaron Patterson to Martin Bosslet

#11 Updated by Koichi Sasada over 1 year ago

ping. status?

#12 Updated by Martin Bosslet over 1 year ago

  • Target version changed from 2.0.0 to next minor

Given the short time until 2.0.0, I believe "next minor" is a more realistic target.

Also available in: Atom PDF