Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: ossl verify cb logic #4423

Closed
wants to merge 5 commits into from
Closed

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Feb 16, 2024

The default X509_STORE_CTX only allows for a single verify callback, but s2n-tls 
sometimes requires multiple pieces of functionality for that callback. Previously we 
did this by chaining function together, but this will become more difficult as we add 
more verify functions that we wish to call.

This commit adds a generic config and callback that allows for verify callbacks to 
be more easily configured.

Description of changes:

This commit adds a new s2n_ossl_verify_cb_config struct on the s2n_x509_validator, which is used to control which verify callbacks are invoked.

This is being done in preparation for the cert key restrictions, which will add yet another verify callback to enforce cert key restrictions.

Call-outs:

Alternate implementation.

Rather than adding a new config, we could instead pass the entire s2n_connection * as the application data, and then use the original checks from s2n_x509_validator_verify_cert_chain to toggle the callbacks,
E.g. instead of

    if (callback_config->disable_time_validation
            && s2n_disable_time_validation_ossl_verify_callback(default_ossl_ret, ctx) == OSSL_VERIFY_CALLBACK_OK) {
        return OSSL_VERIFY_CALLBACK_OK;
    }

We could do

    if (conn->config->crl_lookup_cb
            && s2n_disable_time_validation_ossl_verify_callback(default_ossl_ret, ctx) == OSSL_VERIFY_CALLBACK_OK) {
        return OSSL_VERIFY_CALLBACK_OK;
    }

But duplicating all of the conditional checks felt odd, so I went with the config route.

Testing:

This refactor should not change any behavior, which is confirmed by existing unit tests passing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The default X509_STORE_CTX only allows for a single verify callback, but
s2n-tls sometimes requires multiple pieces of functionality for that
callback. Previously we did this by chaining function together, but this
will become more difficult as we add more verify functions that we wish
to call.

This commit adds a generic config and callback that allows for verify
callbacks to be more easily configured.
@github-actions github-actions bot added the s2n-core team label Feb 16, 2024
@jmayclin jmayclin marked this pull request as ready for review February 16, 2024 21:08
- make verify cb enablement unconditional
- zero struct upon init
- guard ossl value
- remove config, use duplicated checks instead
@jmayclin jmayclin requested a review from goatgoose February 16, 2024 23:28
Comment on lines 590 to 591
return OSSL_VERIFY_CALLBACK_ERROR;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to set an error code of some kind on failure? Or do we just set the return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just set the return value.

- rename ossl callback methods
- switch comment ordering and add justification for callback usage
- add null check
- rename functions
Comment on lines +606 to +607
const struct s2n_connection *conn = (struct s2n_connection *) X509_STORE_CTX_get_app_data(ctx);
if (conn == NULL || ctx == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to check ctx before giving it to the libcrypto in X509_STORE_CTX_get_app_data().

@jmayclin
Copy link
Contributor Author

jmayclin commented Apr 1, 2024

My feature ended up not needing this, and this increases the number of verify cb features we'd be depending on, which is something to avoid if we don't need it.

@jmayclin jmayclin closed this Apr 1, 2024
@jmayclin jmayclin deleted the refactor-verify-cb branch June 15, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants