Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
On 09/06/17 16:24, Bob Beck wrote: effectivelyu providing a limitless OCSP staple is kind of stupid - you may as well simply *not staple* I guess a stapled response without the next_update field set would be treated as valid until the client considers this_update to be too old (for ocspcheck, this seems to be set to 14 days via MAXAGE_SEC). In the case of stapling, I agree that it typically would be much better to use a short period for next_update than not to provide it at all. In my case, I didn't want to use ocspcheck specifically for storing OCSP responses for stapling but in order to check if my local OCSP responder is actually working (i.e., the out-of-band way). In the out-of-band case, clients could also check for freshness by using nonces. During these kinds of tests, I've also noticed that ocspcheck currently only connects to HTTP and HTTPS over their well-known ports which seems to be fine for all public CAs but not necessarily for all local CAs with a corresponding OCSP daemon. In case this lack of flexibility is intended in order to keep the tool simple, I'm also fine with it.
Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
effectivelyu providing a limitless OCSP staple is kind of stupid - you may as well simply *not staple* On Wed, Sep 6, 2017 at 8:23 AM, Bob Beck wrote: > I'm not super inclined to make this "flexible" unless we see this used int > the wild, which I have not. We are more restrictive than > OpenSSL in many areas. > > On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt wrote: > >> On 09/06/17 04:40, Bob Beck wrote: >> >>> Andreas where are you seeing this as being a real issue - who is shipping >>> out OCSP responses without a next update field? >>> >>> >> I've noticed this while playing with a local CA and a corresponding OCSP >> responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is >> optional. If these arguments are not explicitly provided, the next update >> field will not be set. >> >> >> >>> >>> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt >>> wrote: >>> >>> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it always provides a warning and no staplefile is written out. According to RFC 6960, the nextUpdate field is optional. The following patch should handle this case more gracefully and include a suitable debug message only in case -vv is specified. OK? Index: src/usr.sbin/ocspcheck/ocspcheck.c === RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v retrieving revision 1.21 diff -u -p -u -r1.21 ocspcheck.c --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - 1.21 +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size { ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = NULL; const unsigned char **p = (const unsigned char **)&buf; - int status, cert_status=0, crl_reason=0; + int status, cert_status=0, crl_reason=0, next_update=0; time_t now, rev_t = -1, this_t, next_t; OCSP_RESPONSE *resp; OCSP_BASICRESP *bresp; @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size return 0; } if ((next_t = parse_ocsp_time(nextupd)) == -1) { - warnx("unable to parse next update time in OCSP reply"); - return 0; + if (verbose >= 2) + fprintf(stderr, "Optional timestamp for next update not included in OCSP reply\n"); } + else + next_update = 1; /* Don't allow this update to precede next update */ - if (this_t >= next_t) { + if (next_update == 1 && this_t >= next_t) { warnx("Invalid OCSP reply: this update >= next update"); return 0; } @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size /* * Check that next update is still valid */ - if (next_t < now - JITTER_SEC) { + if (next_update == 1 && next_t < now - JITTER_SEC) { warnx("Invalid OCSP reply: reply has expired (%s)", ctime(&next_t)); return 0; @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size vspew("OCSP response validated from %s\n", host); vspew("This Update: %s", ctime(&this_t)); - vspew("Next Update: %s", ctime(&next_t)); + if (next_update == 1) + vspew("Next Update: %s", ctime(&next_t)); return 1; } >>> >> >
Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
I'm not super inclined to make this "flexible" unless we see this used int the wild, which I have not. We are more restrictive than OpenSSL in many areas. On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt wrote: > On 09/06/17 04:40, Bob Beck wrote: > >> Andreas where are you seeing this as being a real issue - who is shipping >> out OCSP responses without a next update field? >> >> > I've noticed this while playing with a local CA and a corresponding OCSP > responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is > optional. If these arguments are not explicitly provided, the next update > field will not be set. > > > >> >> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt wrote: >> >> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it >>> always provides a warning and no staplefile is written out. According to >>> RFC 6960, the nextUpdate field is optional. The following patch should >>> handle this case more gracefully and include a suitable debug message >>> only >>> in case -vv is specified. >>> >>> OK? >>> >>> Index: src/usr.sbin/ocspcheck/ocspcheck.c >>> === >>> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v >>> retrieving revision 1.21 >>> diff -u -p -u -r1.21 ocspcheck.c >>> --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - >>> 1.21 >>> +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - >>> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size >>> { >>> ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd >>> = >>> NULL; >>> const unsigned char **p = (const unsigned char **)&buf; >>> - int status, cert_status=0, crl_reason=0; >>> + int status, cert_status=0, crl_reason=0, next_update=0; >>> time_t now, rev_t = -1, this_t, next_t; >>> OCSP_RESPONSE *resp; >>> OCSP_BASICRESP *bresp; >>> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size >>> return 0; >>> } >>> if ((next_t = parse_ocsp_time(nextupd)) == -1) { >>> - warnx("unable to parse next update time in OCSP reply"); >>> - return 0; >>> + if (verbose >= 2) >>> + fprintf(stderr, "Optional timestamp for next >>> update not included in OCSP reply\n"); >>> } >>> + else >>> + next_update = 1; >>> >>> /* Don't allow this update to precede next update */ >>> - if (this_t >= next_t) { >>> + if (next_update == 1 && this_t >= next_t) { >>> warnx("Invalid OCSP reply: this update >= next update"); >>> return 0; >>> } >>> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size >>> /* >>> * Check that next update is still valid >>> */ >>> - if (next_t < now - JITTER_SEC) { >>> + if (next_update == 1 && next_t < now - JITTER_SEC) { >>> warnx("Invalid OCSP reply: reply has expired (%s)", >>> ctime(&next_t)); >>> return 0; >>> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size >>> >>> vspew("OCSP response validated from %s\n", host); >>> vspew("This Update: %s", ctime(&this_t)); >>> - vspew("Next Update: %s", ctime(&next_t)); >>> + if (next_update == 1) >>> + vspew("Next Update: %s", ctime(&next_t)); >>> return 1; >>> } >>> >>> >>> >> >
Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
On 09/06/17 04:40, Bob Beck wrote: Andreas where are you seeing this as being a real issue - who is shipping out OCSP responses without a next update field? I've noticed this while playing with a local CA and a corresponding OCSP responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is optional. If these arguments are not explicitly provided, the next update field will not be set. On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt wrote: ocspcheck effectively treats a missing nextUpdate like an error, i.e., it always provides a warning and no staplefile is written out. According to RFC 6960, the nextUpdate field is optional. The following patch should handle this case more gracefully and include a suitable debug message only in case -vv is specified. OK? Index: src/usr.sbin/ocspcheck/ocspcheck.c === RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v retrieving revision 1.21 diff -u -p -u -r1.21 ocspcheck.c --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - 1.21 +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size { ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = NULL; const unsigned char **p = (const unsigned char **)&buf; - int status, cert_status=0, crl_reason=0; + int status, cert_status=0, crl_reason=0, next_update=0; time_t now, rev_t = -1, this_t, next_t; OCSP_RESPONSE *resp; OCSP_BASICRESP *bresp; @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size return 0; } if ((next_t = parse_ocsp_time(nextupd)) == -1) { - warnx("unable to parse next update time in OCSP reply"); - return 0; + if (verbose >= 2) + fprintf(stderr, "Optional timestamp for next update not included in OCSP reply\n"); } + else + next_update = 1; /* Don't allow this update to precede next update */ - if (this_t >= next_t) { + if (next_update == 1 && this_t >= next_t) { warnx("Invalid OCSP reply: this update >= next update"); return 0; } @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size /* * Check that next update is still valid */ - if (next_t < now - JITTER_SEC) { + if (next_update == 1 && next_t < now - JITTER_SEC) { warnx("Invalid OCSP reply: reply has expired (%s)", ctime(&next_t)); return 0; @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size vspew("OCSP response validated from %s\n", host); vspew("This Update: %s", ctime(&this_t)); - vspew("Next Update: %s", ctime(&next_t)); + if (next_update == 1) + vspew("Next Update: %s", ctime(&next_t)); return 1; }
Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
On 09/06/17 04:40, Bob Beck wrote: Andreas where are you seeing this as being a real issue - who is shipping out OCSP responses without a next update field? I've noticed this while playing with a local CA and a corresponding OCSP responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is optional. If these arguments are not explicitly provided, the next update field will not be set. On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt wrote: ocspcheck effectively treats a missing nextUpdate like an error, i.e., it always provides a warning and no staplefile is written out. According to RFC 6960, the nextUpdate field is optional. The following patch should handle this case more gracefully and include a suitable debug message only in case -vv is specified. OK? Index: src/usr.sbin/ocspcheck/ocspcheck.c === RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v retrieving revision 1.21 diff -u -p -u -r1.21 ocspcheck.c --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - 1.21 +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size { ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = NULL; const unsigned char **p = (const unsigned char **)&buf; - int status, cert_status=0, crl_reason=0; + int status, cert_status=0, crl_reason=0, next_update=0; time_t now, rev_t = -1, this_t, next_t; OCSP_RESPONSE *resp; OCSP_BASICRESP *bresp; @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size return 0; } if ((next_t = parse_ocsp_time(nextupd)) == -1) { - warnx("unable to parse next update time in OCSP reply"); - return 0; + if (verbose >= 2) + fprintf(stderr, "Optional timestamp for next update not included in OCSP reply\n"); } + else + next_update = 1; /* Don't allow this update to precede next update */ - if (this_t >= next_t) { + if (next_update == 1 && this_t >= next_t) { warnx("Invalid OCSP reply: this update >= next update"); return 0; } @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size /* * Check that next update is still valid */ - if (next_t < now - JITTER_SEC) { + if (next_update == 1 && next_t < now - JITTER_SEC) { warnx("Invalid OCSP reply: reply has expired (%s)", ctime(&next_t)); return 0; @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size vspew("OCSP response validated from %s\n", host); vspew("This Update: %s", ctime(&this_t)); - vspew("Next Update: %s", ctime(&next_t)); + if (next_update == 1) + vspew("Next Update: %s", ctime(&next_t)); return 1; }
Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960
Andreas where are you seeing this as being a real issue - who is shipping out OCSP responses without a next update field? On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt wrote: > ocspcheck effectively treats a missing nextUpdate like an error, i.e., it > always provides a warning and no staplefile is written out. According to > RFC 6960, the nextUpdate field is optional. The following patch should > handle this case more gracefully and include a suitable debug message only > in case -vv is specified. > > OK? > > Index: src/usr.sbin/ocspcheck/ocspcheck.c > === > RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v > retrieving revision 1.21 > diff -u -p -u -r1.21 ocspcheck.c > --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - > 1.21 > +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - > @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size > { > ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = > NULL; > const unsigned char **p = (const unsigned char **)&buf; > - int status, cert_status=0, crl_reason=0; > + int status, cert_status=0, crl_reason=0, next_update=0; > time_t now, rev_t = -1, this_t, next_t; > OCSP_RESPONSE *resp; > OCSP_BASICRESP *bresp; > @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size > return 0; > } > if ((next_t = parse_ocsp_time(nextupd)) == -1) { > - warnx("unable to parse next update time in OCSP reply"); > - return 0; > + if (verbose >= 2) > + fprintf(stderr, "Optional timestamp for next > update not included in OCSP reply\n"); > } > + else > + next_update = 1; > > /* Don't allow this update to precede next update */ > - if (this_t >= next_t) { > + if (next_update == 1 && this_t >= next_t) { > warnx("Invalid OCSP reply: this update >= next update"); > return 0; > } > @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size > /* > * Check that next update is still valid > */ > - if (next_t < now - JITTER_SEC) { > + if (next_update == 1 && next_t < now - JITTER_SEC) { > warnx("Invalid OCSP reply: reply has expired (%s)", > ctime(&next_t)); > return 0; > @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size > > vspew("OCSP response validated from %s\n", host); > vspew("This Update: %s", ctime(&this_t)); > - vspew("Next Update: %s", ctime(&next_t)); > + if (next_update == 1) > + vspew("Next Update: %s", ctime(&next_t)); > return 1; > } > >
[patch] ocspcheck: nextUpdate is optional according to RFC 6960
ocspcheck effectively treats a missing nextUpdate like an error, i.e., it always provides a warning and no staplefile is written out. According to RFC 6960, the nextUpdate field is optional. The following patch should handle this case more gracefully and include a suitable debug message only in case -vv is specified. OK? Index: src/usr.sbin/ocspcheck/ocspcheck.c === RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v retrieving revision 1.21 diff -u -p -u -r1.21 ocspcheck.c --- src/usr.sbin/ocspcheck/ocspcheck.c 8 May 2017 20:15:34 - 1.21 +++ src/usr.sbin/ocspcheck/ocspcheck.c 2 Sep 2017 17:09:00 - @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size { ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd = NULL; const unsigned char **p = (const unsigned char **)&buf; - int status, cert_status=0, crl_reason=0; + int status, cert_status=0, crl_reason=0, next_update=0; time_t now, rev_t = -1, this_t, next_t; OCSP_RESPONSE *resp; OCSP_BASICRESP *bresp; @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size return 0; } if ((next_t = parse_ocsp_time(nextupd)) == -1) { - warnx("unable to parse next update time in OCSP reply"); - return 0; + if (verbose >= 2) + fprintf(stderr, "Optional timestamp for next update not included in OCSP reply\n"); } + else + next_update = 1; /* Don't allow this update to precede next update */ - if (this_t >= next_t) { + if (next_update == 1 && this_t >= next_t) { warnx("Invalid OCSP reply: this update >= next update"); return 0; } @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size /* * Check that next update is still valid */ - if (next_t < now - JITTER_SEC) { + if (next_update == 1 && next_t < now - JITTER_SEC) { warnx("Invalid OCSP reply: reply has expired (%s)", ctime(&next_t)); return 0; @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size vspew("OCSP response validated from %s\n", host); vspew(" This Update: %s", ctime(&this_t)); - vspew(" Next Update: %s", ctime(&next_t)); + if (next_update == 1) + vspew(" Next Update: %s", ctime(&next_t)); return 1; }