-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[SHIRO-740] SslFilter with HTTP Strict Transport Security (HSTS) #55
Conversation
Yeah, you would need to use Though on this topic the filter actually executing is still based on the order filters are defined, and applied to paths. [urls]
/** = ssl
/foobar = ssl
/barfoo = authc
/abcxyz = authc, ssl in the example above the path [main]
# enable hsts
ssl.hsts.enabled = true
# All paths go through ssl filter
priorityFilter.filters = ssl
[urls]
/** = authc That said, this is not part of Shiro at the moment, and I think this pull request can be completed independent of this, but it may help both with your other question: The issue with the path configuration of the ssl filter and path, does seem odd, but I think this solution makes it easier to configure and explain/configure. I'd love to hear other thoughts on this though. |
@bdemers I would like to add this PR to the next release. |
Refer to this link for build results (access rights to CI server needed): |
@fpapon |
@bdemers can I create a Jira for adding the |
Please do! sorry I lost track of this one. I'm going to block out some time in the next week or so to focus on Shiro :) |
|
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.
LGTM
@bdemers can you review please? |
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.
+1
We should probably add this to 1.6 too
We will need to add doc after we cut a release too, I'm assuming the ini
config would look something like:
ssl.hsts.enabled = true
ssl.hsts.includeSubDomains =true
Agree for the doc. |
retest this please |
Hi Brian,
we talked on the mailing list. For anyone not involved here is the summary.
HTTP Strict Transport Security (HSTS) would be a nice addition for all the SSL only sites out there. I think in recent years more and more pages have gone full SSL, with good reasons to do so. It is a bit problematic with SslFilter since this one is path based. If you go HSTS then everything on the site uses https. This might break thinks if you have a path with ssl and one without. You can do that with shiro but not with HSTS.
Some words on the implementation, I am not sure on some things.
I have never used EasyMock before. The test cases in SslFilterTest run fine, but maybe I did it very overcomplicated.
Is
postHandle
invoked even ifisAccessAllowed
returns false? The user agent has to ignore the HTTP header if it is send over HTTP, but I would prefer to send it only over HTTPS. SoisAccessAllowed
needs to return true. I could move the logic to isAccessAllowed, but it looked like the wrong place.