-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Allow specifying a client certificate #112
Conversation
Thanks for making this PR. However, as I've mentioned on the README, I would like to add advanced client cert support, where multiple certs are supported for different sites, and can be generated within the client.
I think adding this smaller support would be nice for some circumstances, but ultimately lead to issues as most people use different certs for different domains, as they should. |
We could update the PR to point to a directory of per-domain certificates in the mean time. Waiting for full in-client UX also sounds good. |
Maybe I shouldn't have closed so prematurely. I think adding multi-cert support at a lower level is a good idea, and the UI can come at a later date, when I have time. I figure this will mostly be a configuration thing, we need a way to specify cert and key files per host (domain and port) in the config file. I don't want to rely on a directory or assumed name schemes (like it will always be named What do you think of this method? [auth]
[auth.certs]
"example.com" = "mycert.crt"
[auth.keys]
"example.com" = "mycert.key" Just off the top of my head. The viper keys would then be |
Sure. I've updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the comments below. One other thing that should be added is a public function (in client.go
):
func HasClientCert(host string) bool
This would return true when there is a valid client cert available for the provided host. I think using the cache only is fine here, it might be slow to attempt to read a few bytes of a file when it's not in the cache.
This doesn't have to be part of this PR, but the reason why I want that func is to disable caching for domains that client certs are being used for. In my early testing of this feature, I found that interactive apps like Astrobotany used redirects to pages to display new content, and it wasn't clear that things had changed/updated, because the cached version was being displayed.
OK, I've updated the PR. |
I would appreciate if you didn't force-push PRs in general, so I can see the diff between commits. Will take a look. |
Thanks a lot for this! |
You're welcome. You can review changes between pushes by clicking on the text force-pushed. |
I'd just like to let you know that I made some changes to this code on the It wasn't a problem when you made this PR, but with the feeds feature there are now goroutines fetching things concurrently, which means that The other change was that from what I can tell, the old code would check every single time for a cert if one was specified for the host but there were errors opening it. Now the code will ignore that cert after the first time getting errors opening it. |
No description provided.