libtls: Support OCSP - Githubissues

markokr commented 9 years ago

Here is support for stapling for both client and server, plus API to launch OCSP network requests.

1) Stapling

Client requests and verifies by default.
Server attaches stapling response, if configured. Config API:

int tls_config_set_ocsp_stapling_file(struct tls_config *cf, const char *fn);
int tls_config_set_ocsp_stapling_mem(struct tls_config *cf, const uint8_t *blob, size_t len);

File is loaded on each request. Some sort of cronjob can be set up to update it periodically. File and mem is not checked in any way, assumes admin has set it up properly. Note that network requests do validate server response.

2) Network API

int tls_ocsp_refresh_stapling(struct tls **ocsp_ctx_p, int *async_fd_p, struct tls_config *config);
int tls_ocsp_check_peer(struct tls **ocsp_ctx_p, int *async_fd_p, struct tls *client);

tls_ocsp_refresh_stapling() loads response over network and attaches to config. Returns TLS_NO_OCSP if cert has no ocsp urls.

tls_ocsp_check_peer() verifies peer cert over network. Returns TLS_NO_OCSP if cert has no OCSP urls set up.

If async_fd_p is unset, they operate in sync mode. If set, a newly created fd will be put there that can be used with any polling method. And they return TLS_WANT_POLLIN / TLS_WANT_POLLOUT as needed. When poll notifies about event, the call needs to be repeated.

In both async and sync mode they create new "struct tls" that can be examined for errors and final OCSP details.

3) Info API.

int tls_get_ocsp_info(struct tls *ctx, int *response_status,
                      int *cert_status, int *crl_reason,
                      time_t *this_update, time_t *next_update, time_t *revoction_time,
                      const char **result_text);

For client connection it returns info about stapled OCSP response. For network requests it returns what OCSP responder sent.

Situations:

No OCSP cert, no stapling from server: returns TLS_NO_OCSP, fills result_text with "no-ocsp"
Have valid OCSP response with good signature - returns 0. All fields are set. This does not mean cert is valid, it can still have revoked status. Or responser may have rejected the request.
OCSP response validation failed, some network error - returns -1. *result_text has verification failure message. If ctx is OCSP network context, it has error message. If client connection to server, it has some later error.

result_text: "no-ocsp" - TLS_NO_OCSP "good", "unknown", "revoked: ..." - successful result "..." - error about failure.

4) Open questions.

How should it coordinate with noverifycert(): check and don't report don't check, or don't even ask? Should the OCSP info be visible or not when noverifycert() is on?

The ->ocsp_result field usage is slightly weird but necessary: it tracks whether there was any OCSP activity at all. ocsp_info is set only on sucessful response. Also, OCSP errors during stapling check will be lost, it will track them.

5) Notes.

Although stapling could work without network requests, the tls_ocsp_refresh_stapling() makes it actually usable.
Main bonus of tls_ocsp_check_peer() is that it makes possible to test the code - both server refresh and actual verification - against normal public servers. So it's worth having on purely testability grounds.
No client ever should make network OCSP request for each new connection. Or server that checks client cert. Instead OCSP responses should be cached, and request is made when cache expires. So the tls_ocsp_check_peer() should not be integrated into basic tls_connect(). libtls could give

bob-beck commented 9 years ago

ohhh marko.. ok - gonna take me a bit to look at this...

bob-beck commented 9 years ago

Marko I see you're doing calls to BIO in there. We would really prefer not to bring that API in for direct use. Can you do this with the conventional socket API?

markokr commented 9 years ago

Well, ATM it handles both http: and https: responders. And uses OCSP http helper functions that use bio.

Unless you have nice http(s) library that could be used here, I think it would be simpler to keep using the libssl helpers.

markokr commented 9 years ago

seems my, uh, essay was cut short:

libtls could give caching api for apps so they don't need to invent it themselves, but longer term solution is to offload network-OCSP and CRL handling to external process via IPC. So if libtls sees non-stapled handshake, it can ask re-validation for external process with stable cache.

One example of such process is Dirmngr. OS X uses similar ocspd

bob-beck commented 9 years ago

I'm pretty reluctant to add anything as complicated as http fetching itself to the library.

Having the mechanisms to deal with it is probably ok.. We need to back and forth a bit over what support needs to be in libtls, versus what should be provided externally.

My two cents is the stuff to do an http request to talk to an ocsp server should not be in there....

On Sun, Sep 13, 2015 at 11:34 PM, Marko Kreen notifications@github.com wrote:

seems my, uh, essay was cut short:

libtls could give caching api for apps so they don't need to invent it themselves, but longer term solution is to offload network-OCSP and CRL handling to external process via IPC. So if libtls sees non-stapled handshake, it can ask re-validation for external process with stable cache.

One example of such process is Dirmngr https://www.gnupg.org/documentation/manuals/dirmngr/. OS X uses similar ocspd https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/ocspd.1.html

— Reply to this email directly or view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-139965798 .

markokr commented 9 years ago

HTTP fetching is already in libcrypto...

But I understand your concern as libtls/libssl maintainer - it's prefereable to keep unnecessary stuff out. But please think also of app developer/user - you are forcing remaining complexity to them. Which means it will not happen...

I think the goal of libtls should be to implement easy-to-use "proper" TLS stack, instead easy-to-use "minimal" stack.

The ideal architecture for fully-featured TLSv1.0...TLSv1.2 stack is indeed keep http-requests out of TLS library and apps, instead you have external system daemon that manages both CRLs and OCSP requests for both peer checking and own stapling updates. Like Dirmngr. But this has not happened [1], and is unlikely to happen. The Dirmngr high point was around 2004 as CRL/OCSP manager for S/MIME. Now it's barely alive as PGP key fetcher for GnuPG.

The "actually working" architecture for near future seems to drop the idea of properly managed CRL and OCSP checks and go with MustStaple [2] / MultiStaple [3] extensions.

Which means libtls has no good way to offload anything and either it implements something or it just does not happen. My conclusion is that network-based OCSP request do have place in libtls - because there is no better place for them.

[1] https://www.grc.com/revocation.htm - test on https://revoked.grc.com/ Chrome/Android/BoringSSL not only have dropped network-based OCSP/CRL checks but also don't bother to check stapled OCSP.

[2] https://tools.ietf.org/html/draft-hallambaker-tlsfeature-10

[3] https://tools.ietf.org/html/rfc6961

bob-beck commented 9 years ago

There are kitchen sinks in libcrapto. The intention of libtls is not to repeat that.

OCSP is currently one of those "really bad ideas" that there is no alternative for some people. Using it on the internet at large is today, a "bad idea".

I think the way we have an external daemon to do it is we write one ;) and we make a sane protocol by which libtls can communicate to one securely.

Alternatively (or in the process of doing this) we can make sure we have the right primitives in libtls that allow someone to implement this. But having http in there is a non starter. this is a tls library, not an http library.

On Mon, Sep 14, 2015 at 4:48 AM, Marko Kreen notifications@github.com wrote:

HTTP fetching is already in libcrypto...

But I understand your concern as libtls/libssl maintainer - it's prefereable to keep unnecessary stuff out. But please think also of app developer/user - you are forcing remaining complexity to them. Which means it will not happen...

I think the goal of libtls should be to implement easy-to-use "proper" TLS stack, instead easy-to-use "minimal" stack.

The ideal architecture for fully-featured TLSv1.0...TLSv1.2 stack is indeed keep http-requests out of TLS library and apps, instead you have external system daemon that manages both CRLs and OCSP requests for both peer checking and own stapling updates. Like Dirmngr. But this has not happened [1], and is unlikely to happen. The Dirmngr high point was around 2004 as CRL/OCSP manager for S/MIME. Now it's barely alive as PGP key fetcher for GnuPG.

The "actually working" architecture for near future seems to drop the idea of properly managed CRL and OCSP checks and go with MustStaple [2] / MultiStaple [3] extensions.

Which means libtls has no good way to offload anything and either it implements something or it just does not happen. My conclusion is that network-based OCSP request do have place in libtls - because there is no better place for them.

[1] https://www.grc.com/revocation.htm - test on https://revoked.grc.com/ Chrome/Android/BoringSSL not only have dropped network-based OCSP/CRL checks but also don't bother to check stapled OCSP.

[2] https://tools.ietf.org/html/draft-hallambaker-tlsfeature-10

[3] https://tools.ietf.org/html/rfc6961

— Reply to this email directly or view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-140036014 .

markokr commented 9 years ago

I agree the the plain OCSP is bad idea. But the stapling is not. So the current goal for libtls should be "How to make easy to set up TLS server with OCSP stapling."

It's possibly for libtls to say to app "you do the http request". But not all apps are http-based... And the OCSP request is not really normal request, eg. redirects really should not be allowed. So I still see that it's simpler for libtls just do the request.

bob-beck commented 9 years ago

I agree with you that it in some situations it it simpler for an application to have something to do a request for it. I just don't agree that it belongs in libtls. this is a higher level api.

This sort of feature creep is what got us into the libcrapto kitchen sink with OpenSSL. We don't want to repeat that mistake.

I think for a starting point, we should try to assume the http request is done by the application, what does it need from libtls to be able to do that.

On Mon, Sep 14, 2015 at 5:32 AM, Marko Kreen notifications@github.com wrote:

I agree the the plain OCSP is bad idea. But the stapling is not. So the current goal for libtls should be "How to make easy to set up TLS server with OCSP stapling."

It's possibly for libtls to say to app "you do the http request". But not all apps are http-based... And the OCSP request is not really normal request, eg. redirects really should not be allowed. So I still see that it's simpler for libtls just do the request.

— Reply to this email directly or view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-140045239 .

markokr commented 9 years ago

App needs url, content-type (application/ocsp-request) and OCSP_REQUEST as der-encoded blob. It needs to do POST req with it, then give response blob back, which libtls needs to verify before using.

markokr commented 9 years ago

OK, here is version 2 of the patch:

removed networking
simplified error handling
tls_set_error_libssl() for places where details from libssl are useful to know.

markokr commented 9 years ago

Client example:

https://github.com/markokr/libtls-tests/blob/master/ocsp-connect.c

bob-beck commented 9 years ago

That's looking more promising - I see you have a whole bunch of guck in there for the ASN1_time stuff.

I have a pending (it's posted to hackers) diff waiting for review that changes a bunch of that stuff. So let's wait a bit.. You should be able to get the times just as a struct tm from libtls shortly.. :)

On Sat, Sep 19, 2015 at 4:10 AM, Marko Kreen notifications@github.com wrote:

OK, here is version 2 of the patch:

removed networking

simplified error handling

tls_set_error_libssl() for places where details from libssl are useful to know.

— Reply to this email directly or view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-141643023 .

markokr commented 9 years ago

Poke poke... The ASN1_TIME changes went in a little while ago... this could be reworked.

bob-beck commented 8 years ago

Poke Poke... the ASN1_TIME changes whent in a little while ago.. this could be reworked

markokr commented 8 years ago

Ok, here is resynced patch. I separated tls_set_error_libssl() to make it clearer.

This patch still contains workaround for #45 as it's unfixed it libressl.

busterb commented 8 years ago

Erg, I was reviewing PRs just now, and I'm pretty sure all of us completely missed your updates :(

bob-beck commented 8 years ago

oooh. i missed that he had updated this too illl have a peek

On Sunday, 31 January 2016, Brent Cook notifications@github.com wrote:

Erg, I was reviewing PRs just now, and I'm pretty sure all of us completely missed your updates :(

— Reply to this email directly or view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-177495990 .

bob-beck commented 8 years ago

I'm in your PR marko.. playing with your stuff

bob-beck commented 8 years ago

we can remove buggy-verify now

bob-beck commented 8 years ago

Marko, I've made this work with -current OpenBSD, and posted a diff (with your test progtram) to tech@openbsd.org so we can start reviewing it.. there will probably be a few things to change and suggestions.

One of the things I noticably removed from it before posting was the addition of tls_error_libssl()... I see where you're going with that, but it's kind of a separate issue of whether or not we want that, and I don't want that public API addition confusing the issue (i.e. I wanna keep this strictly to OCSP support) . I'm perfectly ok with that coming back as a separate things saying "Hey I want a thing that appends my stuff to an underlying ssl library error", but I would like to keep that separate for now.

bob-beck commented 8 years ago

I'm targeting this to go in post OpenBSD 6.0 release with some changes

busterb commented 8 years ago

I think Bob is working on an updated version now, discussion will move to tech@openbsd.org. I'm going to close this PR as-is since it was damaged from the recent CVS tree updates.

bob-beck commented 8 years ago

While this has been closed, the code is now in-tree and should show up in the next portable release.

markokr commented 7 years ago

Thanks for merging it.

I agree with leaving tls_error_libssl() out, it should come as whole tree-change for all situations where libssl's 'reason' contains additional details about failure that would be useful for user to see.

But I noticed you have changed two TLS_OCSPRESPONSE codes from values I picked up from RFC6960 section 4.2.1. The comment references section 2.3 but that does not contain any values.

Is that an oversight or is there a reason behind that change?

bob-beck commented 7 years ago

likely an oversight on my part. let me take a quick look

On Tue, Dec 27, 2016 at 09:29 Marko Kreen notifications@github.com wrote:

Thanks for merging it.

I agree with leaving tls_error_libssl() out, it should come as whole tree-change for all situations where libssl's 'reason' contains additional details about failure that would be useful for user to see.

But I noticed you have changed two TLS_OCSPRESPONSE codes from values I picked up from RFC6960 section 4.2.1. The comment references section 2.3 but that does not contain any values.

Is that an oversight or is there a reason behind that change?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-269340033, or mute the thread https://github.com/notifications/unsubscribe-auth/AHv2dYV5e9gYnbuyHlNCqF_yw7WDyrvLks5rMS4dgaJpZM4F8cvY .

bob-beck commented 7 years ago

Heh.. definately an overaggresive cleanup on either joel's or my part.. I have the obvious fix being reviewed.. thanks

On Thu, Dec 29, 2016 at 4:17 PM, Bob Beck beck@obtuse.com wrote:

likely an oversight on my part. let me take a quick look

On Tue, Dec 27, 2016 at 09:29 Marko Kreen notifications@github.com wrote:

Thanks for merging it.

I agree with leaving tls_error_libssl() out, it should come as whole tree-change for all situations where libssl's 'reason' contains additional details about failure that would be useful for user to see.

But I noticed you have changed two TLS_OCSPRESPONSE codes from values I picked up from RFC6960 section 4.2.1. The comment references section 2.3 but that does not contain any values.

Is that an oversight or is there a reason behind that change?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/libressl-portable/openbsd/pull/47#issuecomment-269340033, or mute the thread https://github.com/notifications/unsubscribe-auth/AHv2dYV5e9gYnbuyHlNCqF_yw7WDyrvLks5rMS4dgaJpZM4F8cvY .

libressl / openbsd

libtls: Support OCSP #47