Skip to content

Commit e406790

Browse files
committed
Properly handle protected endpoints with query strings.
To facilitate nginx configuration, we've changed the redirect from https://$http_host/saml/login?url=$request_uri to https://$http_host/saml/login$request_uri. I erroneously thought $request_uri would be encoded, but it wasn't. The old way created a potentially invalid uri. The new way preserves the original url that we want to come back to.
1 parent 41d812e commit e406790

File tree

5 files changed

+107
-16
lines changed

5 files changed

+107
-16
lines changed

README.md

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@ URL, the proxy will take care of the rest.
77
Example nginx.conf would look like the following...
88

99
```
10-
location /secure {
11-
auth_request /saml/status/group/uw_example_group;
10+
location / {
11+
auth_request /saml/status/group/uw_it_all;
1212
auth_request_set $auth_user $upstream_http_x_saml_user;
13+
error_page 401 = @login_required;
1314
proxy_set_header Remote-User $auth_user;
1415
proxy_pass http://secure:5000/;
1516
}
@@ -18,21 +19,23 @@ location /saml/ {
1819
proxy_set_header Host $http_host;
1920
proxy_set_header X-Forwarded-Proto $scheme;
2021
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
21-
proxy_set_header X-Forwarded-Prefix /saml;
2222
proxy_set_header X-Saml-Entity-Id https://samldemo.iamdev.s.uw.edu/saml;
23+
# acs - post-back url registered with the IdP.
2324
proxy_set_header X-Saml-Acs /saml/login;
2425
proxy_pass http://saml:5000/;
2526
}
2627
27-
location @error401 {
28-
return 302 https://$http_host/saml/login?url=$request_uri;
28+
location @login_required {
29+
return 302 https://$http_host/saml/login$request_uri;
2930
}
3031
```
3132

33+
See the [example nginx config](test/nginx/server.conf) for more examples.
34+
3235
## SECRET_KEY
3336

3437
This app wants an environment variable `SECRET_KEY`, which should be a secure,
3538
randomly-generated string. Otherwise, we generate one on the fly, which only
36-
works long as the app is running, and won't work in a distributed environment.
39+
works as long as the app is running, and won't work in a distributed environment.
3740
SECRET_KEY is used to sign cookies, so setting a new key effectively
3841
invalidates all existing sessions.

app.py

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,35 +29,58 @@ def status(group=None):
2929
403 if a group was requested that the user is not a member of, or 200
3030
if the user is authenticated.
3131
32-
group - a UW Group the user must be a member of.
32+
group - a UW Group the user must be a member of. An SP must be registered
33+
to receive that group.
3334
"""
3435
userid = session.get('userid')
3536
groups = session.get('groups', [])
3637
if not userid:
3738
abort(401)
3839
if group and group not in groups:
40+
message = f"{userid} not a member of {group} or SP can't receive it"
41+
app.logger.error(message)
3942
abort(403)
4043
headers = {'X-Saml-User': userid,
4144
'X-Saml-Groups': ':'.join(groups)}
4245
txt = f'Logged in as: {userid}\nGroups: {str(groups)}'
4346
return Response(txt, status=200, headers=headers)
4447

4548

49+
def _saml_args():
50+
"""Get entity_id and acs_url from request.headers."""
51+
return {
52+
'entity_id': request.headers['X-Saml-Entity-Id'],
53+
'acs_url': urljoin(request.url_root, request.headers['X-Saml-Acs'])
54+
}
55+
56+
57+
@app.route('/login/')
58+
@app.route('/login/<path:return_to>')
59+
def login_redirect(return_to=''):
60+
"""
61+
Redirect to the IdP for SAML initiation.
62+
63+
return_to - the path to redirect back to after authentication. This and
64+
the request.query_string are set on the SAML RelayState.
65+
"""
66+
query_string = '?' + request.query_string.decode()
67+
if query_string == '?':
68+
query_string = ''
69+
return_to = f'/{return_to}{query_string}'
70+
args = _saml_args()
71+
return redirect(uw_saml2.login_redirect(return_to=return_to, **args))
72+
73+
4674
@app.route('/login', methods=['GET', 'POST'])
4775
def login():
4876
"""
49-
Return a SAML Request redirect, or process a SAML Response, depending
50-
on whether GET or POST.
77+
Process a SAML Response, and set the uwnetid and groups on the session.
5178
"""
5279
session.clear()
53-
args = {
54-
'entity_id': request.headers['X-Saml-Entity-Id'],
55-
'acs_url': urljoin(request.url_root, request.headers['X-Saml-Acs'])
56-
}
5780
if request.method == 'GET':
58-
args['return_to'] = request.args.get('url', None)
59-
return redirect(uw_saml2.login_redirect(**args))
81+
return login_redirect()
6082

83+
args = _saml_args()
6184
attributes = uw_saml2.process_response(request.form, **args)
6285

6386
session['userid'] = attributes['uwnetid']
@@ -80,5 +103,7 @@ def logout():
80103
def healthz():
81104
"""Return a 200 along with some useful links."""
82105
return '''
83-
<p><a href="login">Sign in</a></p><p><a href="logout">Logout</a></p>
106+
<p><a href="login">Sign in</a></p>
107+
<p><a href="status">Status</a></p>
108+
<p><a href="logout">Logout</a></p>
84109
'''

docker-compose.yml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
version: '3'
2+
services:
3+
saml:
4+
build: .
5+
image: nginx-saml-proxy
6+
volumes:
7+
- ./app.py:/app/app.py
8+
nginx:
9+
build: test/nginx
10+
image: mynginx
11+
ports: ["443:443"]
12+
volumes:
13+
- ./test/nginx/server.conf:/etc/nginx/conf.d/server.conf

test/nginx/Dockerfile

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
FROM nginx as certbuilder
2+
3+
RUN apt-get update && apt-get install -y ssl-cert && apt-get clean
4+
RUN mkdir /ssl && \
5+
cp -p /etc/ssl/private/ssl-cert-snakeoil.key /ssl/key.pem && \
6+
cp -p /etc/ssl/certs/ssl-cert-snakeoil.pem /ssl/cert.pem
7+
8+
FROM nginx
9+
10+
EXPOSE 80
11+
EXPOSE 443
12+
13+
RUN rm /etc/nginx/conf.d/* && mkdir /static
14+
COPY server.conf /etc/nginx/conf.d/server.conf
15+
COPY --from=certbuilder /ssl /etc/nginx/ssl
16+
17+
CMD ["nginx", "-g", "daemon off;"]

test/nginx/server.conf

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
server {
2+
listen 443 ssl;
3+
ssl_certificate /etc/nginx/ssl/cert.pem;
4+
ssl_certificate_key /etc/nginx/ssl/key.pem;
5+
root /usr/share/nginx/html;
6+
7+
# anyone with a UW NetID can access this
8+
location / {
9+
auth_request /saml/status;
10+
auth_request_set $auth_user $upstream_http_x_saml_user;
11+
error_page 401 = @login_required;
12+
}
13+
14+
# user must be a member of uw_it_all
15+
location /secure {
16+
auth_request /saml/status/group/uw_it_all;
17+
error_page 401 = @login_required;
18+
alias /usr/share/nginx/html;
19+
}
20+
21+
location /saml/ {
22+
proxy_set_header Host $http_host;
23+
proxy_set_header X-Forwarded-Proto $scheme;
24+
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
25+
proxy_set_header X-Saml-Entity-Id https://samldemo.iamdev.s.uw.edu/saml;
26+
proxy_set_header X-Saml-Acs /saml/login;
27+
proxy_pass http://saml:5000/;
28+
}
29+
30+
location @login_required {
31+
return 302 https://$http_host/saml/login$request_uri;
32+
}
33+
}

0 commit comments

Comments
 (0)