Expose a few more OpenSSL functions that are useful for DTLS support by njsmith · Pull Request #6138 · pyca/cryptography (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

njsmith

Also OP_NO_RENEGOTIATION I guess, which is useful because renegotiation is the worst.

@njsmith

This was referenced

Jun 26, 2021

reaperhulk

@njsmith

...This thing where each CI run has to be re-approved again is pretty annoying. I guess feel free to add me to the org temporarily? I promise that the only way I will abuse my power would be to kick myself out again ASAP.

@alex

An easier move, more compatible with how we manage permissions generally, would be if you could send some other small PR (docs fix, typo, etc.) and then we could merge that, freeing this up to be run CI jobs without our intervention.

@njsmith

it's probably not very effective

@njsmith

@njsmith

Well, I guess I'll keep an eye out for typos then? :-) In the mean time, I think it should pass now, so probably it will only take another 3 attempts...

@njsmith

Well would you look at that. I guess this is ready for review then.

reaperhulk

@@ -594,6 +614,10 @@
long (*DTLS_get_link_min_mtu)(SSL *) = NULL;
#endif
#if CRYPTOGRAPHY_OPENSSL_LESS_THAN_111
size_t (*DTLS_get_data_mtu)(SSL *) = NULL;

Choose a reason for hiding this comment

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

Take a look in _conditional.py to see what we do when a symbol isn't actually available.

Choose a reason for hiding this comment

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

It looks like currently, _conditional.py doesn't actually remove any conditionally-available functions (e.g. DTLS_get_link_min_mtu, visible just above my new code in the diff, doesn't appear there). So I made my best guess at how it should work for DTLS_get_data_mtu -- take a look.

Choose a reason for hiding this comment

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

Ha, I would argue that is a bug (we don't want callables that are null func ptrs!). Looks like that's a thing we need to separately fix for the DTLS bindings though...

@njsmith

@alex

Looks like there's some flake8 issues.

@njsmith

@njsmith

Huh. Codecov output is a bit mangled, but I think it might be saying that in _comparisons.py, we're never detecting DTLS_get_data_mtu as being falsey? That doesn't make sense to me -- we're definitely setting it to NULL on libressl and openssl 1.1.0, and I confirmed locally that CFFI treats null function pointers as falsey (bool(ffi.cast("void (*)(void)", 0)) == False).

@njsmith

AFAICT it works fine in CFFI's ABI mode, but I can't figure out how to do it in the API mode.

@njsmith

image

alex

alex approved these changes Jun 30, 2021

vishwin pushed a commit to vishwin/py-cryptography that referenced this pull request

Dec 23, 2022

@njsmith @vishwin

…yca#6138)

it's probably not very effective

AFAICT it works fine in CFFI's ABI mode, but I can't figure out how to do it in the API mode.

aeeaan pushed a commit to aeeaan/cryptography that referenced this pull request

Aug 11, 2024

@njsmith @aeeaan

…yca#6138)

it's probably not very effective

AFAICT it works fine in CFFI's ABI mode, but I can't figure out how to do it in the API mode.