Create new config section for web security settings #2815
Conversation
|
Open question: where in the documentation should this be documented? I am tempted to make a new "security"-page. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2815 +/- ##
=======================================
Coverage 57.14% 57.15%
=======================================
Files 567 568 +1
Lines 41268 41277 +9
=======================================
+ Hits 23584 23593 +9
Misses 17684 17684 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Open question: where in the documentation should this be documented? I am tempted to make a new "security"-page.
Not sure we have a suitable pre-existing page or section in the Sphinx docs, so I can understand your temptation.
Our most basic form of documenting config options is, however, a comment immediately preceding the option in the example config file, as evidenced by other options already present in webfront.conf. You should add that as a minimum.
Also, changes or additions to config files usually warrant a note in NOTES.rst, to make users aware that they may need to change something.
Naming the option tls does not seem intuitive to me. Going from just the name, without documentation, it looks to me like an option to switch TLS on or off, something NAV cannot do, since this is up to the web server you set up to serve NAV.
|
I'm thinking we'll need a security-section in the proper docs eventually as we cannot expect the c-suite to read code :) |
There was a problem hiding this comment.
Slightly better.
Still need the release notes tip and the doc change. If we put off making a "security" docs section, we should at least mention this new option wherever the docs mention SSL (there are one or two notes in the various installation guides about adding SSL as being an exercise left up to the reader)
hmpf
force-pushed
the
secure-cookie
branch
2 times, most recently
from
c56c7d3
to
57dd64d
Compare
There was a problem hiding this comment.
Docs are good!
Really sorry, but I still can't get comfortable with the option name. If all the new option really affects in NAV is turning on secure cookies, then I think it should be named "use_secure_cookies" or something like that. (after all, naming things is still one of the hardest CS problems :P )
Or even just reuse the setting name from Django :D |
Right now all the flag does is turn on secure cookies, yes. There are other cookie-flags that also depend on SSL/TLS being enabled (CSRF-protection for one, which we ought to start using), and IIRC other features that depend on SSL/TLS being enabled. I want to avoid having multiple flags, just an easy way to tell Django that it can assume it is running under SSL/TLS. I've looked into automatically detecting whether we're running under SSL/TLS but that can't be used to change settings. (Changing settings for an already running Django is a no-no.) |
Alright, but then a |
hmpf
force-pushed
the
secure-cookie
branch
2 times, most recently
from
ef341bb
to
a43c302
Compare
Well it's for subsets of NAV that needs tls, so...
It's a better name than a django setting specific one yes. I started with "ssl" because I wanted to avoid a multi-part name since there seems to be no consensus on how to spell multi word config flags: with spaces, '-', '_', camel case, no gaps at all... |
Well, if you really want it short and sweet, then I'm fine with just |
Eh, I quite like "needs_tls". We could have a clean-up round where we standardize on one system per config-file though. Oh, actually, it is possible to make such a flag turn on tls, for django code (though leaving it to django is not recommended). That's not compatible with how we deploy though and people can have their own localsettings anyway if they need that. |
|
Closes #2194
Use it (for now) for setting secure cookies and the most basic form of XSS-protection.