Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrading crewjam/saml from v0.3.0 to v0.4.5 #169

Merged
merged 8 commits into from
Jun 3, 2021
Merged

Upgrading crewjam/saml from v0.3.0 to v0.4.5 #169

merged 8 commits into from
Jun 3, 2021

Conversation

gchamon
Copy link

@gchamon gchamon commented May 7, 2021 •
edited
Loading

to:
cc: @subspacecommunity/subspace-maintainers
related to:
resolves: #167

Background

There was a severe vulnerability in crewjam/saml v0.3.0 that allowed bypassing SAML SSO. Updating to v0.4.5 would solve the issue.

The way SAML works in subspace is to basically split the handler procedure encoded in samlSP.RequreAccount. This function is supposed to make it easy to use SAML SSO, however it requires the use of pure net/http. As we use julienschmidt/httprouter. We have to handle it ourselves.

The way this is done is basically split samlSP.RequreAccount in two. The first part is implemented in the two handlers ssoHandler and samlHandler. ssoHandler, in turn handles specifically the authentication flow. Therefore, we must reimplement it using samlSP.HandleStartAuthFlow. The code in it is almost the same as in samlSP.RequreAccount. The difference is that samlSP. ServeHTTP is reserved for samlHandler.

The other modification was the refactoring of the procedure that extracts the subject name from the JWT Token. This changed quite drastically in 0.4.x. This version did away with the convenient function GetAuthorizationToken. Now we have to extract the session from the conext and use type assertion to cast it to JWTSessionClaims. Without it, we would be left with sessionWithAttributes.GetAttributes which in turn only returns the contents of attrs of the JWT Token. This cast restores the access to what was previously retrieved with token.StandardClaims. Now we access it with jwtSessionClaim directly, which is the complete decoded jwt token.

DISCLAIMER: I am NOT a security expert and the first contact I ever had with golang was applying this fix. So I welcome everyone to treat this pull request as an invitation for debating, so that we can arrive at an optimal solution.

Changes

  • Updated crewjam/saml from v0.3.0 -> v0.4.5
  • Applied necessary refactor to account for breaking changes.

Testing

  • Build and start a fresh container
  • Log in with an admin account
  • Create a testing account on jumpcloud
  • Configure SSO as explained in the documentation
  • Log in with the created user in jumpcloud

@gchamon gchamon mentioned this pull request May 7, 2021
Closed
@gchamon gchamon mentioned this pull request May 7, 2021
Merged
@metalcated
Copy link

I’ll gladly test but need a walk through on how to put this all together. I use the docker image.

@gchamon
Copy link
Author

gchamon commented May 9, 2021 •
edited
Loading

@metalcated I also use docker for convenience. This would be done so:

  • clone my repo fork: git clone https://github.com/waycarbon/subspace
  • checkout the branch that originates this pr git checkout fix/upgrade-saml
  • Optional: if you want, you can add the flag --debug to entrypoint.sh where subspace cmd is being called. The debug messages will show up in the file /etc/service/subspace/log/main/current inside the container. It is mapped outside the container for convenience in the docker-compose.yml.
  • from there you build subspace image as you prefer. I prefer to use docker-compose for convenience, so just put the following file in the root directory and issue docker-compose build, remembering to change SUBSPACE_HTTP_HOST to suit your need (localhost for testing in the same machine, or as in my case, the local IP address of the target machine):
version: "3.3"
services:
  subspace:
    image: subspace-local
    build: .
    # image: subspacecommunity/subspace
    container_name: subspace
    volumes:
     - ./subspace-data:/data
     # - ./subspace-log:/etc/service/subspace/log
    restart: always
    environment:
     - SUBSPACE_HTTP_HOST=192.168.0.4
     - SUBSPACE_LETSENCRYPT=false
     - SUBSPACE_HTTP_INSECURE=true
     - SUBSPACE_HTTP_ADDR=":80"
     - SUBSPACE_NAMESERVERS=1.1.1.1,8.8.8.8
     - SUBSPACE_LISTENPORT=51820
     - SUBSPACE_IPV4_POOL=10.99.97.0/24
     - SUBSPACE_IPV6_POOL=fd00::10:97:0/64
     - SUBSPACE_IPV6_NAT_ENABLED=1
    cap_add:
     - NET_ADMIN
    network_mode: "host"
  • build the container: docker-compose build
  • start the container: docker-compose up -d Note: running in detach mode because the actual logs from the container are near useless. We will follout the actual output with the following command:
  • follow the logs with tail -f ./subspace-log/main/current
  • proceed to configure subspace with an admin account
  • there you should be able to configure the SSO I used jumpcloud because there is a free tier that lets you create a SAML SSO application, so if you want to do the same, just follow the instructions here. Dont forget to also create a user and add that user to the app. After setting up subspace with the SAML metadata, you should be able to download subspace SAML metadata and upload it to your app. In a nutshell:
    • Create app in jumpcloud
    • download jumpcloud metadata and upload to subspace
    • download subspace metadata and upload it to subspace
    • double check if IdP entity ID is correct. In my case it was http://192.168.0.4/
    • remember to add users to the sso app
  • now you can logout from admin and try to login using SSO

Tests I can think of:

  • login and logout using SSO
  • creating a config
  • making SSO user admin
  • deleting SSO user using admin account

Edit
mapping the logs folder with volumes doesn't work. What you would have to do to access the logs is enter the running container with bash and follow the file there:

docker exec -it subspace bash
# then inside the container
tail -f /etc/service/subspace/log/main/current

This was referenced May 9, 2021
Closed
Closed
asabirov
asabirov approved these changes May 10, 2021
View reviewed changes
cmd/subspace/web.go Outdated
@@ -190,10 +190,17 @@ func WebHandler(h func(*Web), section string) httprouter.Handle {

// Needs a new session.
if samlSP != nil {
if token := samlSP.GetAuthorizationToken(r); token != nil {
r = r.WithContext(samlsp.WithToken(r.Context(), token))
session, _ := samlSP.Session.GetSession(r)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to handle this error too? We can log it if it's difficult to integrate it into the current user flow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as singin will be required at the end of the middleware flow, we could just log the error for future reference and continue, because session will be nil anyways. Or do you think it would be better to explicitly redirect to /signin?

subspace/cmd/subspace/web.go

Lines 237 to 238 in e3fc811

logger.Warnf("auth: sign in required")
web.Redirect("/signin")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added logging to the error, however, now every time we start the container it logs an error. I will change it to debug instead of error

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@metalcated
Copy link

metalcated commented May 11, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 11, 2021 •
edited
Loading

@metalcated I just tested with Okta, using a newly created free account and I successfully logged into subspace. Here is what I did:

  • Creating the okta app
    image
    image
    image
    image
    image
    image

Note: you have to set name ID and application username to use the email address. Subspace works by validating users by email. Also, change 192.168.0.4 to whatever the URL of your instance is. In my case, I also used http because I am testing inside a private network.

  • download okta metadata
    image

  • Paste its contents to subspace metadata field

Hope it helps!

@metalcated
Copy link

metalcated commented May 11, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 11, 2021

Not sure why it doesn't work for me but consistently get "forbidden". I did validate the settings and even paste in the new SAML metadata. This was setup and working for a couple years but just simply is not working. Maybe AD integration causes it not to work? I will create a whole new account and try later on.
…
On Tue, May 11, 2021 at 6:58 AM Gabriel Chamon Araujo < @.***> wrote: @metalcated https://github.com/metalcated I just tested with Okta, using a newly created free account and I successfully logged into subspace. Here is what I did: - Creating the okta app [image: image] https://user-images.githubusercontent.com/9471861/117804371-14ac2d80-b22e-11eb-9e47-a598c2740a6b.png [image: image] https://user-images.githubusercontent.com/9471861/117804404-1fff5900-b22e-11eb-8496-c7bc355e3828.png [image: image] https://user-images.githubusercontent.com/9471861/117804436-28f02a80-b22e-11eb-9682-79d2464cba41.png Note: you have to set name ID and application username to use the email address. Subspace works by validating users by email. Also, change 192.168.0.4 to whatever the URL of your instance is. In my case, I also used http because I am testing inside a private network. - download okta metadata [image: image] https://user-images.githubusercontent.com/9471861/117804737-81bfc300-b22e-11eb-9c73-8cc5fc61948b.png - Paste its contents to subspace metadata field — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#169 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKRQCUCFZAPXEU2O4R7QV3TNEEUHANCNFSM44LLGGWA .

I have little experience with the actual process of federating user login, and less so with Okta. Sorry I cannot be more of help. But a couple of ideas:
Have you tried with a freshly created app?
Have you tried with a fresh subspace container, without its data?

@gchamon
Copy link
Author

gchamon commented May 11, 2021

@metalcated one last thing I forgot. You must add users to your Okta SSO app otherwise they wont be allowed to access to application. You do it by adding the users in the Assignments tab in your application:

image

@metalcated
Copy link

metalcated commented May 11, 2021 via email

@metalcated
Copy link

metalcated commented May 11, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 12, 2021

Just deployed in production where I work. Zero problems so far with Azure AD.

@asabirov
Copy link

Works fine with Google Workspace. Good job!

@agonbar
Copy link
Collaborator

agonbar commented May 16, 2021

@metalcated did you manage to get this to work? I would like to have this merged. The problem with SAML is that is very hard to integrate in something like a Unit Test, so setting up environments to test changes in a clean space for every PR is a bit hard

@gchamon
Copy link
Author

gchamon commented May 16, 2021 •
edited
Loading

@agonbar I have a terraform module for subspace that works well, but needs documentation. It could serve to create disposable environments for these types of experiments.

https://github.com/waycarbon/terraform-aws-subspace

@metalcated
Copy link

metalcated commented May 16, 2021 via email

@metalcated
Copy link

metalcated commented May 17, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 17, 2021

@metalcated Lets try that

@metalcated
Copy link

metalcated commented May 17, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 17, 2021

@metalcated I am getting error 1020, Access Denied from cloudflare

@metalcated
Copy link

metalcated commented May 17, 2021 via email

@gchamon
Copy link
Author

gchamon commented May 17, 2021

@metalcated do you think there could be anything we could do on our part to support Cloudflare?

@metalcated
Copy link

I think we could do something differently to auto generate SSL certificates that do work with Cloudflare. Possibly allow the usage of externally generated certs which can be configured via docker-compose.yml or something to that effect.

@metalcated
Copy link

@metalcated I am getting error 1020, Access Denied from cloudflare

You can try again, I unblocked Brazil.

@gchamon
Copy link
Author

gchamon commented May 17, 2021

I think we could do something differently to auto generate SSL certificates that do work with Cloudflare. Possibly allow the usage of externally generated certs which can be configured via docker-compose.yml or something to that effect.

I believe this would be a new feature request, with a different issue worth investigating.

@gchamon gchamon requested a review from a team May 19, 2021 21:20
@agonbar
Copy link
Collaborator

agonbar commented Jun 3, 2021

Well, the cloudflare certificates problem can go to another PR, so after some tests, I'm merging this.

Thanks to everyone for all the work!

@agonbar agonbar merged commit 018495d into subspacecommunity:master Jun 3, 2021
@gchamon gchamon deleted the fix/upgrade-saml branch June 3, 2021 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asabirov asabirov asabirov approved these changes

@agonbar agonbar agonbar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

saml@0.3.0 vulnerability: Authentication Bypass
4 participants
@gchamon @metalcated @asabirov @agonbar