Skip to content

Conversation

@Ralith
Copy link
Collaborator

@Ralith Ralith commented Jan 13, 2020

Fixes #595.

@github-actions
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.531% when pulling 62eeb55 on server-name into 44267ec on master.

self.0.lock().unwrap().inner.protocol().map(|x| x.into())
}

/// The server name specified by the client
Copy link
Member

Choose a reason for hiding this comment

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

Should we replace these methods with a crypto_session() accessor at the quinn layer, as well? I feel like abstracting over this isn't working out so well for us.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's appealing, but because quinn::Connection is Send + Clone, access to the interior of a proto::Connection must always involve a lock, so we can't expose a reference directly, and returning a lock guard is both a bit weird and a substantial footgun as it would block other operations on the connection.

We could do with_session<T>(&self, f: impl FnOnce(&S) -> T) -> T but that's also pretty weird. Conversely, even a hypothetical cryptographic protocol that doesn't support ALPN and/or SNI can harmlessly return None for them, so this approach seems the most practical. I'm open to other ideas, though.

We could still get rid of the accessors on proto::Connection, including in the crypto traits, and rely on the already-hardcoded dependency on TLS in the high level API, but we'd need to expose more of proto::crypto::rustls for that to be viable, as get_sni_hostname is defined only on rustls::ServerSession, not rustls::Session, and references to the former cannot currently be constructed externally.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, fair point. Not sure all that is more appealing than what we've got today...

@djc djc merged commit 8f31992 into master Jan 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the server-name branch January 13, 2020 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How would one handle SNI with multiple domains?

3 participants