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

Getting client certificate #783

Merged
merged 15 commits into from
Jan 8, 2020
Merged

Conversation

kazu-yamamoto
Copy link
Contributor

--
-- Since 3.3.5
clientCertificate :: Request -> Maybe CertificateChain
clientCertificate = fromMaybe (throw (userError "getClientCertificate")) . Vault.lookup getClientCertificateKey . vault
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for using a bottom value here instead of propagating the Maybe? If we need to distinguish different cases, I'd rather a specific sum type to represent the different ways things can fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. It's from copy-and-paste. Does the following code make sense?

clientCertificate :: Request -> Maybe CertificateChain
clientCertificate = join . Vault.lookup getClientCertificateKey . vault

Copy link
Member

Choose a reason for hiding this comment

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

Yup, seems reasonable.

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. I pushed this. Can we merge this PR now?

@snoyberg
Copy link
Member

I think the version bump has broken all of CI. Do you want me to take a crack at fixing that before we merge?

@kazu-yamamoto
Copy link
Contributor Author

No rush. Now I think we should wait for getting tls 1.5.3 released.

@kazu-yamamoto
Copy link
Contributor Author

New versions of tls and tls-session-manager have been released.
@snoyberg Would you try to fix CI?

@snoyberg
Copy link
Member

snoyberg commented Jan 7, 2020

@kazu-yamamoto did the package that provides the Data.X509 module change? I'm seeing this:

warp               > /home/vsts/work/1/s/warp/Network/Wai/Handler/Warp/Types.hs:11:1: error:
warp               >     Could not load module ‘Data.X509’
warp               >     It is a member of the hidden package ‘x509-1.7.5’.
warp               >     Perhaps you need to add ‘x509’ to the build-depends in your .cabal file.
warp               >     Use -v to see a list of the files searched for.
warp               >    |

I've pushed my attempted CI fixes to this branch.

@kazu-yamamoto
Copy link
Contributor Author

I compared v1.5.2 and v1.5.3 but there is no change for exporting. Very strange.

@ocheron Any thoughts?

@kazu-yamamoto
Copy link
Contributor Author

@snoyberg f828294 added x509 as dependency. That's why?

@ocheron
Copy link

ocheron commented Jan 8, 2020

Apparently it's needed in the benchmark module too but wasn't added.

@kazu-yamamoto
Copy link
Contributor Author

@ocheron Wow! Nice catch! Thanks. Hoping that CI will recover.

@kazu-yamamoto
Copy link
Contributor Author

@snoyberg Should we also add tls-session-manager-0.0.4 to extra-deps:?

@snoyberg
Copy link
Member

snoyberg commented Jan 8, 2020

Yes, good call. I've pushed a bunch of updates on the branch, including updates to the stack.yaml files and a lower bound in warp-tls. I've also improved the build matrix a bit by dropping 8.0.2 (no longer supported) and adding in 8.8.1 testing.

@snoyberg
Copy link
Member

snoyberg commented Jan 8, 2020

@kazu-yamamoto woohoo, looks like CI passed!

kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this pull request Jan 8, 2020
@kazu-yamamoto kazu-yamamoto merged commit 35a8624 into yesodweb:master Jan 8, 2020
@kazu-yamamoto kazu-yamamoto deleted the client-cert branch January 8, 2020 10:43
@kazu-yamamoto
Copy link
Contributor Author

@snoyberg Thanks! I have merged this PR.

@kazu-yamamoto
Copy link
Contributor Author

I have released new versions of warp and warp-tls.

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.

Support not only requesting client certificates but inspecting them once a connection established
3 participants