Implements a TOFU approach to tls certs #41
No reviewers
Labels
No Label
blocked
bug
build
documentation
duplicate
enhancement
finger
gemini
gopher
help wanted
http
in progress
invalid
local
needs-info
non-code
non-functional
non-urgent
question
release
rendering
suggestion
telnet
terminal
urgent
wontfix
No Milestone
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sloum/bombadillo#41
Loading…
Reference in New Issue
No description provided.
Delete Branch "tofu"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I am not really happy with the flow of things in the retrieve method of the gemini.go file. I also think that in theory the TofuDigest stuff could be in its own module and imported into both gemini and main as needed. Not sure if it is necessary, but it might help futureproof some things. Right now gemini is the only protocol we are dong this with, but if new things were to pop up we'd be more flexible with it in its own subpackage.
I also have not updated the
Fetch
method to use this system.What I have done is verify that this is working more or less as intended. If I visit a gemini site I have not been to, the cert gets hashed and added to my
.bombadillo.ini
keyed to the host. When I visit the site again, this is checked against as best as I can tell. We are still catching non-matching hostnames (thank youzaibatsu.circumlunar.space
) and all fo the file io is working pretty well. I also installed lolcat, to make reading the ini more fun ;)I'll comment out some sections of code that need attention.
It is late-ish and I need to crash, but I added some comments to hopefully guide things a little bit here.
@ -62,3 +62,3 @@
func (b Bookmarks) IniDump() string {
if len(b.Titles) < 0 {
if len(b.Titles) < 1 {
I just happened to see this. The logic was flawed, the length of a slice will never be less than 0. What was meant was == 0 or < 1 (I went with the later, but if there is a strong opinion I can update to the former). This, as well as a few other things in this pr, ended up in the pr accidentally by me seeing something and fixing it rather than brnching and fixing... but then not wanting to git reset and redo changes.
< 1 seems to fit fine in this context. This might be something you feel differently about in the morning.
@ -834,2 +834,3 @@
case "gemini":
capsule, err := gemini.Visit(u.Host, u.Port, u.Resource)
capsule, err := gemini.Visit(u.Host, u.Port, u.Resource, &c.Certs)
go saveConfig()
Each time we visit a gemini site there is the potential for an updated certificate. I should probably move this after the error check now that I think about it, but I stand by using a goroutine for it. I dont think there should be any filelock issues... though I am not certain. I'll do some reading.
The error should be checked immediately after you do something that might cause an error, right?
This is the first time I've seen goroutines. I like it!
For
saveConfig()
to have a file lock error, wouldn't you have to browse gemini sites faster than the config could be written? Is it possible?If anything, it seems like the error returned by
saveConfig()
is not being checked or handled when called by the goroutine.(I'll continue reviewing tomorrow)
Yeah. The error should def come first. I'll update that right after this.
Goroutines are really powerful/cool. I like the way they are implemented. Yeah, for there to be a filelock issue gemini would have to be browsed really fast. Oh, that is true. That the error is silenced that way. Hmmm.... something to think about there. In theory it shouldnt be the end of the world to not display an error for this, as if the cert isnt saved, a new one will be grabbed on the next program run and hopefully save at that point... but that isnt great.
I may be able to set a boolean flag for writing to that file and try to set up some kind of control structure. I definitely dont want that file write to be blocking if it doesnt have to be.
@ -24,8 +24,8 @@ type Config struct {
Bookmarks struct {
Titles, Links []string
}
Colors []KeyValue
I went a different way with colors allowing for inversion rather than full coloring. People can still light and dark mode this way, and it just uses their terminal's natural color pallet for foreground and background (plus inversion of that).
This is a good approach, I like the way these themes look.
@ -43,1 +111,4 @@
// Verify that the handshake ahs completed and that
// the hostname on the certificate(s) from the server
// is the hostname we have requested
Old comments. I will update and try to comment out the mess that follows as well.
@ -44,0 +119,4 @@
hostCertExists := td.Exists(host)
matched := false
for _, cert := range connState.PeerCertificates {
This for loop I am really not in love with the internals of... It feels like a mess. It does seem to work, but there is likely a better way. If you/anyone thinks one out, I would LOVE for it to be a good starting point, but not the ending point.
I see what you mean, but am not too sure how it could be improved.
In the situation where a host cert is recorded because you've visited the site previously, but then that host gets a new cert - maybe the old one expired - will it get to line 124 and return false and prevent the site from loading?
Just to comment further on the internals - it could just be that it is a bit hard to follow because it's doing a fair few different things each loop. The process requires a lot of checking to work properly. Switch statements can make things like this easier to read.
Also I find the term 'exists' like in
hostCertExists
a bit confusing. It means that the host has a certificate already recorded in the configuration file, right?I agree a switch could improve this. Doing separate loops for separate conditionals could take away some of the nested conditionals inside the loop as well. It owuld mean duplicating the loop, but looping through certs should be pretty cheap as I have been assured that there should never really be more than one for gemini servers, two or three at the absolute most (that opinion comes from solderpunk).
Would something like
hostIsKnown
orpreviouslyVisited
be better? I definitely want to give the sense that a boolean should be expected for the value and want to tie it tot he fact that a certificate is already held by Bombadillo for that host.As to the changing host cert question. Yes, at present the site would be prevented from loading. I am unclear on what the behavior should be at that point. It doesn't make sense to just grab a new one, as we are explicitly trying to prevent someone getting to a site for which there is a bad certificate. and if we just grab a new one when certs dont match or are expired then we diminish the security as a man in the middle could just provide a bad cert with a matching hostname... we'd see no match and then just grab a new one?
Maybe a special case for expiration is in order? So that if the cert expired we know to try to grab a new one? The only problem I see there is if the server stops offering the expired cert, we have no way to know from the hash that it expired and it will always just look like the site doesnt have a valid/matching cert. I'll have to read more about TOFU and see how these cases should be handled. At present, it takes the most cautious method possible.
I do want to add a new command
:clear [hostname]
or maybe:clearcert [hostname]
that would allow a user to clear out a certificate. This seems like something a system should do. It can allow the user to shoot themselves in the foot, but at least gives them the ability to control their system to their liking.@ -38,3 +38,3 @@
func MakeUrl(u string) (Url, error) {
var out Url
re := regexp.MustCompile(`^((?P<scheme>gopher|telnet|http|https|gemini)://)?(?P<host>[w-.d]+)(?::(?P<port>d+)?)?(?:/(?P<type>[01345679gIhisp])?)?(?P<resource>.*)?$`)
re := regexp.MustCompile(`^((?P<scheme>[a-zA-Z]+)://)?(?P<host>[w-.d]+)(?::(?P<port>d+)?)?(?:/(?P<type>[01345679gIhisp])?)?(?P<resource>.*)?$`)
This should not have been in this PR, but I went too far with it and didnt want to turn back. This just makes it so we'll allow any text for the scheme. It will get caught as an unsupported scheme later. This makes it so we dont have to do case insensitivity for only one part of the regex (the scheme)... which would be really slow. Plus the or for the different schemes was less than great anyway.
@ -67,3 +63,4 @@
return out, fmt.Errorf("no host")
}
out.Scheme = strings.ToLower(out.Scheme)
A bunch of things in the client want a lowercase scheme, so doing it here makes sense rather than all over the place later.
@ -93,0 +87,4 @@
}
if out.Resource == "" || out.Resource == "/" {
out.Mime = "1"
}
I just realized that the first if could be an or condition on the second. :-/ shoulda noticed that.
I made a few big changes tot he way tofu is handled. Most of the looping now lives in the digest methods.
Note that I also updated the delimiter for hashing, so you will need to clear the certs out of your
~/.bombadillo.ini
file before using or they will not match anything.@ -19,0 +56,4 @@
return "", fmt.Errorf("Invalid hostname, no key saved")
}
func (t *TofuDigest) Match(host string, cState *tls.ConnectionState) error {
@asdf The match method has been redone to handle the loop for matching instead of jsut comparing certs. It will search through the PeerCertificates slice and find something or not and respond appropriately.
In the event that a certificate has expired,
t.newCert
is called to see if the host is offering another certificate that is valid (matches hsotname and is within active/expiry window).This looks nice and I like that it can handle expiration.
@ -19,0 +83,4 @@
return fmt.Errorf("No matching certificate was found for host %q", host)
}
func (t *TofuDigest) newCert(host string, cState *tls.ConnectionState) error {
@asdf This is a new method that will look at the available PeerCertificates and try to find a valid one to save as a certificate. If it cannot find a valid certificate it will respond with an error rather than nil. If it finds one, it adds it.
@ -43,1 +149,4 @@
connState := conn.ConnectionState()
// Begin TOFU screening...
This section is now commented and feels much cleaner to me. Let me know what you think. This feels like it can work. It allows for getting a new certificate in the event of an expired one.
The only issue left is if a host no longer offers the expired certificate and just changed their certificate. I believe that is where the
:clear [host]
command can come into play. If someone keeps getting invalid or no matching certificate, they can clear and try again if they want. Which is semi-inline with ssh (which notifies you of a difference and asks if you'd like to proceed). Both due to the way messaging is structured for bombadillo and my preferred approach to security, I think the way implemented here works well. It blocks things that have behaved weirdly and leaves it to a user that knows what they are doing to move forward and make that choice actively instead of just hittingok
every time a certificate warning comes up.Yeah I really like this change and the way this flows now.
I haven't too much experience with certificate handling, but this approach seems the most reasonable for this model. It should be supported by user-friendly information, but not something they might just disregard. I dislike the onus being put completely on the user to understand why something doesn't work.
@ -229,0 +369,4 @@
for i, data := range hash {
hex[i] = []byte(fmt.Sprintf("%02X", data))
}
return fmt.Sprintf("%s", string(bytes.Join(hex, []byte(":"))))
I changed the delimiter here to
:
to be more in line with what people are used to seeing in ssh. You may need to clear out the certs you have from your~/.bombadillo.ini
file to be able to view those sites now that the hashing has changed.On the topic of 'exists', I noticed it is the term used on wikipedia so maybe I'm just not familiar with it. Maybe this function can be commented to say "checks if the hostname has a certificate that exists in the certificate store."
But it seems very easy to read now that I understand what it is doing. The rest of these receivers have similar styles of names, all in the context of the certificate store. So I'm not sure anymore lol
With this last change, Bombadillo is exiting with no error and return code 0 when you try to browse to a gemini site that has a cert recorded.
If anything, maybe this error could indicate why the certs were not valid
Ok. Repull. I got it fixed. I had a structure where I was doing:
Because err could be nil, if it hit that second part of the conditional there was a panic because err has no method
Error
in that case. I rewrote it in a more sensible way. Not sure why I didnt hit that as an issue while I was going through it this morning.In addition to fixing that I added a command:
purge
(also aliased top
).The syntax is:
:purge [hostname]
It will take a wildcard to purge all:
purge *
The testing that I have done to verify it catches bad matches is as follows:
purge *
~/.bombadillo.ini
in an editor and change a value in the hash for the gemini site you visited, save and close the editorpurge [host name]
(where the brackets/hostname are the host name of the gemini site you have been visiting)I have not been able to test an expired hash, but I believe the concept of how it is handled to be sound.
Let me know if that all works for you or if you encounter any issues.
This works fine for me.
I also notice that some of the sites that previously did not work are now working (like zaibatsu.circumlunar.space) but there are some with cert issues still (like carcosa.net). Do you have any thoughts about working around sites that have cert issues?
I know solderpunk over at the zaibatsu generated a new cert that had a matching hostname (after I let them know about the invalid cert). For carcosa.net, I'm not quite sure what to do about that. I mean, the idea of security is that if it is not secure you don't do it... I kind of feel like that site is black until they fix their certificates. :-/
I dont know though, I can see the other side of the argument. At the moment though I feel like Bombadillo's stance on security is a hard one in which there is no override when there is a bad certificate. Otherwise, what is the point of having the security at all? I'm definitely open to suggestions though. The purge certs thing was meant to help alleviate some issues with certs without completely compromising security... but it doesnt help if the host just does not have a valid certificate to offer...
Do you know who runs carcosa.net? I'd be happy to reach out to them about their cert and see if it can get fixed.
Of the servers zaibatsu.circumlunar.space lists, this is the status of everything:
VALID: gemini.conman.org
VALID: zaibatsu.circumlunar.space
VALID: tilde.black
VALID: tilde.pink
VALID: vger.cloud
VALID: yam655.com
VALID: heavysquare.com
VALID: mozz.us
VALID: typed-hole.org
BAD CERT: carcosa.net
CONNECTION REFUSED: dgold.eu
CONNECTION REFUSED: consensus.circumlunar.space
Ok that makes a lot of sense, and I agree with a strict approach to this.
Awesome. Unless you can think of something else that needs to be done here, this can probably get merged into
develop
:)I think the next problem to tackle will be client certificates being passed to servers. Once that is done I think we'll be fairly feature complete for gemini, which will leave distribution. I have read a TON lately about various packaging methods, makefiles, installers, autotools, etc. I have a bunch of thoughts, but I think it makes more sense to open two issues and move discussions there:
I'll open those. Let me know if you think anything else needs to be done on this pr before being merged in.
I think this is all good for this PR. I'll let you merge it in at your convenience.