Skip to content

Expose client-supplied server name #596

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 13, 2020
Merged

Expose client-supplied server name #596

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions quinn/src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,13 @@ impl Connection {
self.0.lock().unwrap().inner.protocol().map(|x| x.into())
}

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

@djc djc Jan 13, 2020

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

@Ralith Ralith Jan 13, 2020

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

@djc djc Jan 13, 2020

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...

///
/// Returns `None` for outgoing connections.
pub fn server_name(&self) -> Option<String> {
self.0.lock().unwrap().inner.server_name().map(|x| x.into())
}

// Update traffic keys spontaneously for testing purposes.
#[doc(hidden)]
pub fn force_key_update(&self) {
Expand Down