feat: add HSTSMaxAge, HSTSIncludeSubdomains, and configurable ciphers#232
Conversation
- adds HSTS middleware - makes cipher suites configurable - TODO: instead of defining ciphers iterate over what go has
Make uppercase of HSTS and TLS consistent in YAML and config. Also add test with incorrect config to validate that wrong cipher names are detected by config parser.
not very clean yet, mostly generated and fixed rough edges. Needs to be refactored dry
- update docs - update example config
|
@slapcat : feel free to add your manual testing results or provide additional review. |
|
LGTM! I confirmed everything still works without these options configured and that the HSTS headers appear in requests when configured. |
gaborbk
left a comment
gaborbk
left a comment
There was a problem hiding this comment.
Thank you for this PR! I have a few minor comments, otherwise looks great :)
alesstimec
left a comment
alesstimec
left a comment
There was a problem hiding this comment.
LGTM, but i'd suggest returning an error, when an error is returned from parseCipheSuites
| cipherSuites, err := parseCipherSuites(c.TLSCipherSuites) | ||
| if err != nil { | ||
| logger.Errorf("cannot parse cipher suites: %s", err) | ||
| return nil |
There was a problem hiding this comment.
are you sure we don't want to return an error here? might be surprising to some that they set a set cipher suites that then isn't used, because one of them was misspelled. i propose returning an error so that the user can deal with it.
There was a problem hiding this comment.
IMO introducing an error here would mean introducing it above as well (when the certificate is created). I did not want to change too much of the existing code which is why I followed the pattern of returning nil in that case. That said, I'd like to get it going as is first. But I'm happy to follow up with something later.
- brush up some comments - remove commented code - remove maxAge check in middleware - make HSTS string constants, fix max-age format - remove debug print statements in test
Co-authored-by: Gabor Borics-Kuerti <gabor.borics.kuerti@canonical.com>
4 participants
Description
Add settings for
HSTSMaxAgeand configurable ciphers.Motivated by customer requests to the MAAS team.
Engineering checklist
Check only items that apply
Test instructions
It should be tested that
HSTS-max-ageandHSTS-include-subdomainsinjected correctly if given (included in config test)Notes for code reviewers
Grill me, my first bigger code contribution in Go ;)