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

bound session cache #195

Merged
merged 3 commits into from
Apr 8, 2024
Merged

bound session cache #195

merged 3 commits into from
Apr 8, 2024

Conversation

ehaydenr
Copy link
Contributor

When establishing new TLS sessions, servers may send multiple session tickets (RFC8446 4.6.1). hyper-boring caches tickets without placing a limit on how many tickets are cached. This leads to unbounded growth of hyper-boring's cache and leaves clients vulnerable to malicious servers who might send many session tickets to exhaust a client's available memory.

This change bounds the cache to a default of 8 tickets.

When establishing new TLS sessions, servers may send multiple session
tickets (RFC8446 4.6.1). hyper-boring caches tickets without placing a
limit on how many tickets are cached. This leads to unbounded growth of
hyper-boring's cache and leaves clients vulnerable to malicious servers
who might send many session tickets to exhaust a client's available
memory.

This change bounds the cache to a default of 8 tickets.
///
/// The session cache configuration of `ssl` will be overwritten. Session capacity is per
/// session key (domain).
pub fn with_connector_and_capacity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make it future-proof and use settings struct here:

struct HttpsLayerSettings {
    session_cache_capacity: usize
}

and change this to:

pub fn with_connector_and_settings(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing it that way adding a field to HttpsLayerSettings is still a breaking change so it's not really future-proof. We may want a builder instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated!

@ehaydenr ehaydenr requested a review from inikulin December 1, 2023 16:53
@ehaydenr ehaydenr requested a review from nox December 4, 2023 20:08
@Noah-Kennedy
Copy link

@inikulin @nox what's the blocker to merging this?

@nox
Copy link
Collaborator

nox commented Apr 7, 2024

I think there was a discussion about what should the default max, but the discussion didn't reach a conclusion. Honestly I think we should just merge it.

@Noah-Kennedy
Copy link

I'm in agreement here - let's just merge.

@Noah-Kennedy
Copy link

I don't have permissions to do so, so this will need to be handled by someone else.

@rushilmehra rushilmehra merged commit 870ccd9 into cloudflare:master Apr 8, 2024
23 checks passed
@rushilmehra
Copy link
Collaborator

Merged, thanks @ehaydenr

@ehaydenr ehaydenr deleted the sessioncache branch April 15, 2024 14:02
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.

5 participants