Skip to content

Notes while reading #2

Open
Open
Notes while reading#2

Description

@njsmith
njsmith
opened on Jun 15, 2017

Not sure this is the most useful way to record these, but I figure it's better than my just thinking them to myself and then forgetting about them :-). Let me know if they should be moved to some other repo or mailing list or something.

  • IMO the SNI callback should receive the hostname as a bytes object containing the A-label version of the hostname, which for the stdlib wrapper means there should be a call to .encode("idna") in pep543_callback.

  • OpenSSL has an annoying thing where when you set a new context from the SNI callback, it only updates some parts of its internal configuration from that context, while ignoring others. Possibly the PEP 543 wrapper callback should work around this by noticing when these other things have changed and updating them manually, if it can, and otherwise error out. (ref)

  • We need at least two kinds of TLSError: one that means "definitely an error", and one that means "definitely an error, unless you're running in HTTP-compatibility mode, in which case pretend it's a clean EOF". (For the stdlib wrapper, SSLEOFError should give you the first one, and everything else the second one.) [Edit: I see that PEP 543 says that's RaggedEOF; so then the note here is that _error_converter needs to do SSLEOFError → RaggedEOF.]

  • I'm pretty sure the stdlib never raises SSLZeroReturnError. Though I can't prove it. But it certainly tries to convert it into a return b"" internally.

  • Writing to a ssl.MemoryBIO definitely never returns short. The API allows for the possibility because it might happen for other BIOs, but MemoryBIOs in particular will grow to whatever size they need to, so the nervous comment in receive_from_network is unnecessary.

  • You do need to watch out though that MemoryBIO.write(b"") "helpfully" gets interpreted as indicating an EOF, which may or may not match your API convention for receive_from_network.

  • Speaking of unbounded memory buffers, it's also impossible to get an SSLWantWriteError. I think we should consider removing it from the PEP 543 abstract API.

  • You might want to enforce that do_handshake is called given https://bugs.python.org/issue30141

Activity

  1. njsmith commented on Jun 16, 2017

    @njsmith
    njsmith
    on Jun 16, 2017
    MemberAuthor

    A few more thoughts:

    • PEP 543 should probably be very explicit about what happens on the frontend side if you try to call read/write before calling do_handshake. And I guess the answer should probably be "you get an error". (The other option is "the backend magically does the handshake for you". Either can work, the stdlib does a third thing that's broken, and probably we want this in the spec so we don't have some backends picking one option and other backends picking the other.)

    • PEP 543's shutdown semantics are currently way underspecified. There are two things the user might want to do (a) send close_notify and then move on with their life, or (b) send close_notify and then keep reading until they get a close_notify back (or timeout or whatever). OpenSSL provides SSL_shutdown, which sort of munges them together into one function call, which does something like (assuming a non-blocking BIO here):

      if close_notify_not_sent:
    send_close_notify()
    return bool(close_notify_received)
else:
    process_incoming_bio()
    if close_notify_received:
        return True
    else:
        signal SSL_WANT_READ

      Then there's the stdlib unwrap method, which does something like:

      if close_notify_not_sent:
    send_close_notify()
process_incoming_bio()
if not close_notify_received:
    raise SSLWantReadError

      The stdlib semantics are actually pretty reasonable, so long as you know what they are – if you just want to send close_notify and move on then you call it once and ignore the WantReadError, or otherwise you keep calling it like any other non-blocking SSL function. But I'm guessing that the other backends will have native semantics that are different again, so, definitely needs to be specified :-).

      (If this becomes complicated then I guess it'd also be reasonable for the first version to only support "send close_notify and move on", since there are very few cases where you need to wait for a close_notify after sending one.)

    • I wonder if the bytes_buffered/peek/consume-style API is overkill? Trio, Twisted, asyncio all want to pull out all the bytes every time, I believe. So maybe this buffering code should move into the shared TLSWrappedSocket implementation, instead of being reimplemented inside each backend? (And I'm not sure, but I think for the case of TLSWrappedSocket + the stdlib ssl backend, there are actually more copies this way than if TLSWrappedSocket did its own buffering?) I don't have a strong opinion either way here, just noticed this.

  2. Lukasa commented on Jun 16, 2017

    @Lukasa
    Lukasa
    on Jun 16, 2017
    Member

    Speaking of unbounded memory buffers, it's also impossible to get an SSLWantWriteError. I think we should consider removing it from the PEP 543 abstract API.

    No, because wrapped sockets.

    I wonder if the bytes_buffered/peek/consume-style API is overkill?

    Yeah, the answer is "maybe". But the reasoning for it is simple: to reduce the number of places that have to buffer. That may be unnecessary, in the end, of course.

  3. njsmith commented on Jun 16, 2017

    @njsmith
    njsmith
    on Jun 16, 2017
    MemberAuthor

    No, because wrapped sockets.

    Oh, sure. But maybe we should drop it from the set of allowable exceptions raised by the abstract sans-IO interface.

  4. Lukasa commented on Jun 16, 2017

    @Lukasa
    Lukasa
    on Jun 16, 2017
    Member

    @njsmith BTW, where there are things that are underspecified it'd be super-useful to see a PR that would better specify them. I'm acutely aware that I've basically written all of this document by myself, which means it's seen a substantial lack of coverage in areas that aren't popping up in my mind.

    (Also, if we rely on me doing all of this it'll take ages to get done. ;) )

  5. njsmith commented on Jun 16, 2017

    @njsmith
    njsmith
    on Jun 16, 2017
    MemberAuthor

    Where would such a PR go? directly against the peps repository?

  6. Lukasa commented on Jun 17, 2017

    @Lukasa
    Lukasa
    on Jun 17, 2017
    Member

    Hrm, yeah, good point. I have an in-progress copy of the PEP somewhere: I'll add it to this repo.

  7. Lukasa commented on Jun 19, 2017

    @Lukasa
    Lukasa
    on Jun 19, 2017
    Member

    Ok I've brought in an updated copy of PEP 543. Feel free to make PRs against it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions