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

Allow specifying a client certificate #112

Merged
merged 1 commit into from Nov 4, 2020
Merged

Allow specifying a client certificate #112

merged 1 commit into from Nov 4, 2020

Conversation

tslocum
Copy link
Contributor

@tslocum tslocum commented Nov 4, 2020

No description provided.

@makew0rld
Copy link
Owner

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.

@makew0rld makew0rld closed this Nov 4, 2020
@tslocum
Copy link
Contributor Author

tslocum commented Nov 4, 2020

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.

@makew0rld
Copy link
Owner

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 thedomain.com.crt) so that certs can be shared across clients.

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 "auth.certs." + host and "auth.keys." + host.

@makew0rld makew0rld reopened this Nov 4, 2020
@tslocum
Copy link
Contributor Author

tslocum commented Nov 4, 2020

Sure. I've updated the PR.

Copy link
Owner

@makew0rld makew0rld left a 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.

client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
@tslocum
Copy link
Contributor Author

tslocum commented Nov 4, 2020

OK, I've updated the PR.

@makew0rld
Copy link
Owner

I would appreciate if you didn't force-push PRs in general, so I can see the diff between commits. Will take a look.

@makew0rld makew0rld merged commit 4e26a51 into makew0rld:master Nov 4, 2020
@makew0rld
Copy link
Owner

Thanks a lot for this!

makew0rld added a commit that referenced this pull request Nov 4, 2020
@tslocum
Copy link
Contributor Author

tslocum commented Nov 4, 2020

You're welcome. You can review changes between pushes by clicking on the text force-pushed.

@tslocum tslocum deleted the clientcert branch November 4, 2020 22:00
@makew0rld
Copy link
Owner

I'd just like to let you know that I made some changes to this code on the feeds branch, in e026eb2. You can track the progress of the feeds feature overall in #61.

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 certCache needs to be protected with a mutex. I would occasionally get concurrent map writes panic.

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.

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.

None yet

2 participants