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

TLS session manager #630

Merged
merged 9 commits into from
Jul 4, 2017
Merged

Conversation

kazu-yamamoto
Copy link
Contributor

I would like to include an in-memory TLS session manager to wai rather than to tls because it requires auto-update and it is used in warp-tls.

I'm implementing this, rather than the mechanism of stateless session tickets, based on this analysis and Single-Use Tickets. In short, I would like to provide anti-replay features.
Please read the documentation of this package (i.e. the beginning of the SesssionManagers.hs).

This package can be used for TLS 1.2 or earlier, hence this PR includes the modification of warp-tls. This is also used for TLS 1.3 which is implemented currently as one of my topic branch.

Any suggestions are welcome.

@kazu-yamamoto kazu-yamamoto requested a review from snoyberg June 29, 2017 06:39
@kazu-yamamoto
Copy link
Contributor Author

@ocheron You might be interested in this.

@kazu-yamamoto
Copy link
Contributor Author

@snoyberg Please hold merging until a new version of psqueues is released.
See jaspervdj/psqueues#18

@kazu-yamamoto kazu-yamamoto mentioned this pull request Jun 29, 2017
15 tasks
@ocheron
Copy link

ocheron commented Jun 29, 2017

You might be interested in this.

Session resumption is certainly desirable and the analysis you reference is really interesting.

If I understand correctly, security concerns come only when using session tickets or 0-RTT, which we don't have currently in tls. Session manager API could have two functions sessionResume and sessionResumeOnlyOnce, so that sessions are resumed multiple times when safe.

@kazu-yamamoto
Copy link
Contributor Author

@ocheron Thank you for your suggestions.
That's exactly what I'm wondering now.
I will consider this today.

@kazu-yamamoto
Copy link
Contributor Author

@ocheron Your idea is really cool. I will implement sessionResumeOnlyOnce in my TLS 1.3 branch for tls.


----------------------------------------------------------------

-- | Creating a in-memory session manager.
Copy link
Member

Choose a reason for hiding this comment

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

s/a/an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

cons :: Int -> Item -> DB -> DB
cons lim (k,t,v,Add) db
| Q.size db == lim = case Q.minView db of
Nothing -> Q.insert k t v Q.empty -- not happens, just in case
Copy link
Member

Choose a reason for hiding this comment

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

How about adding an assert False here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you refer to Control.Exception.assert?
If so, what the second argument should be?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I meant. I'd leave the second argument as the current right-hand side of the -> as a fair fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks and done.


clean :: DB -> IO (DB -> DB)
clean olddb = do
currentTime <- getCurrentTime
Copy link
Member

Choose a reason for hiding this comment

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

It may be better to use a monotonic clock from the clock package to avoid clock skew issues.

(And yes, that same advice probably applies to some of my reaper logic elsewhere like http-client.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What constructor of Clock should I choose for getTime for this purpose?
Realtime?

Copy link
Member

Choose a reason for hiding this comment

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

I've typically used Monotonic

@@ -129,6 +131,15 @@ data TLSSettings = TLSSettings {
-- Default: Nothing
--
-- Since 3.2.2
, tlsSessionManagerConfig :: Maybe SM.Config
-- ^ Configuration for in-memory TLS session manager.
-- If Nothing, 'TLS.noSessionManager' is used.
Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't we default to using some basic configuration? Is there a downside to turning on a manager by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • It changes the current behavior of warp-tls (i.e. allowing session resumptions).
  • It requires more memory than the current.

So, I think that users should explicitly set this parameter.

@kazu-yamamoto kazu-yamamoto force-pushed the tls-session-manager branch from a11beb4 to 821b6f0 Compare July 4, 2017 01:17
@kazu-yamamoto
Copy link
Contributor Author

@snoyberg A new psqueues has been released.
I fixed all issues and rebased this branch for merging.
Please review this again and merge this PR.

@kazu-yamamoto kazu-yamamoto force-pushed the tls-session-manager branch from 821b6f0 to f68fe53 Compare July 4, 2017 01:48
@kazu-yamamoto
Copy link
Contributor Author

I have removed the patch for warp-tls so that all tests should pass in CI.

@snoyberg snoyberg merged commit 703fd8a into yesodweb:master Jul 4, 2017
@kazu-yamamoto
Copy link
Contributor Author

Thank you!

@snoyberg
Copy link
Member

snoyberg commented Jul 4, 2017 via email

@kazu-yamamoto kazu-yamamoto deleted the tls-session-manager branch July 4, 2017 02:56
@kazu-yamamoto
Copy link
Contributor Author

sessionResumeOnlyOnce is being implemented in the tls13-18 branch of my tls repo.

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.

3 participants