SoarinFerret commented 4 years ago

Describe the bug I believe I have uncovered two issues when setting up SAML with ADFS, not sure if I need to supply multiple bug reports or not.

1) ADFS requires both requests and responses for SLS to be signed by the SP. I was able to get around this by updating the saml config file located in app/config/saml2.php and providing the certificate as an environment variable.

...
        // sp config under onelogin
        'sp' => [
...
            'x509cert' => env('SAML2_SP_x509',''),
            'privateKey' => env('SAML2_SP_PRIVATEKEY',''),
        ],
...
        // New section under onelogin called 'security', below idp
        'security' => [
            'logoutRequestSigned' => true,
            'logoutResponseSigned' => true,
        ],

2) The probably bigger issue is that the SLS request sent to ADFS does not seem to make much sense. The nameid format should be returned the same as Bookstack requests it, which is urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress. However, Bookstack is sending back urn:oasis:names:tc:SAML:2.0:nameid-format:entity. In addition, the NameID should be my UPN / E-Mail address, but instead it appears to be sending an ADFS URL.

Here is the sample response:

<samlp:LogoutRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                     xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
                     ID="ONELOGIN_c83ad4ce9dab4d3a2f510f1ca01a8f78ac0ed4a2"
                     Version="2.0"
                     IssueInstant="2020-02-22T17:37:07Z"
                     Destination="https://sts.example.com/adfs/ls/"
                     >
    <saml:Issuer>https://bookstack.example.com/saml2/metadata</saml:Issuer>
    <saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">http://sts.example.com/adfs/services/trust</saml:NameID>
</samlp:LogoutRequest>

Here is the error generated on ADFS after receiving that:

The SAML Single Logout request does not correspond to the logged-in session participant. 
Requestor: https://bookstack.example.com/saml2/metadata 
Request name identifier: Format: urn:oasis:names:tc:SAML:2.0:nameid-format:entity, NameQualifier:  SPNameQualifier: , SPProvidedId:  
Logged-in session participants: 
Count: 1, [Issuer: https://bookstack.example.com/saml2/metadata, NameID: (Format: urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress, NameQualifier:  SPNameQualifier: , SPProvidedId: )]  

This request failed.

Steps To Reproduce Steps to reproduce the behavior:

Sign in with ADFS
Click on Logout

Expected behavior I believe I should be expecting 2 things:

The ability to specify options to require signed logout requests (via an env var or something)
For the nameid format during the SLS to be the same format and return the correct var.

Your Configuration (please complete the following information):

Exact BookStack Version (Found in settings): v0.28.2
PHP Version: 7.3.14
Hosting Method (Nginx/Apache/Docker): Linuxserver.io Docker Container

Additional context Add any other context about the problem here.

Here is my .env relevant SAML config:

## SAML Config
# Set authentication method to be saml2
AUTH_METHOD=saml2

# Set the display name to be shown on the login button.
# (Login with <name>)
SAML2_NAME=ADFS

# Name of the attribute which provides the user's email address
SAML2_EMAIL_ATTRIBUTE=mail

# Name of the attribute to use as an ID for the SAML user.
SAML2_EXTERNAL_ID_ATTRIBUTE=http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn

# Name of the attribute(s) to use for the user's display name
# Can have mulitple attributes listed, separated with a '|' in which
# case those values will be joined with a space.
# Example: SAML2_DISPLAY_NAME_ATTRIBUTES=firstName|lastName
# Defaults to the ID value if not found.
SAML2_DISPLAY_NAME_ATTRIBUTES=displayName

# Identity Provider entityID URL
SAML2_IDP_ENTITYID=https://sts.example.com/federationmetadata/2007-06/federationmetadata.xml

# Auto-load metatadata from the IDP
# Setting this to true negates the need to specify the next three options
SAML2_AUTOLOAD_METADATA=true

# Custom values I added on my own
SAML2_SP_x509="-----BEGIN CERTIFICATE-----..."
SAML2_SP_PRIVATEKEY="-----BEGIN PRIVATE KEY-----..."

And here are my ADFS claims rules:

@RuleTemplate = "LdapClaims"
@RuleName = "User Attributes"
c:[Type == "http://schemas.microsoft.com/ws/2008/06/identity/claims/windowsaccountname", Issuer == "AD AUTHORITY"]
 => issue(store = "Active Directory", types = ("http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn", "mail", "groups", "displayName"), query = ";userPrincipalName,otherMailbox,tokenGroups,displayName;{0}", param = c.Value);

@RuleName = "Transform UPN to Name ID"
c:[Type == "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn"]
 => issue(Type = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier", Issuer = c.Issuer, OriginalIssuer = c.OriginalIssuer, Value = c.Value, ValueType = c.ValueType, Properties["http://schemas.xmlsoap.org/ws/2005/05/identity/claimproperties/format"] = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress");

SoarinFerret commented 3 years ago

I was recently reached out to for help with this, I figured I would update this issue. I actually have gotten this configuration working without using SLS. More details can be found here: https://blog.kanto.cloud/bookstack-adfs-setup/

The gist of of it is choosing to manually define your parameters for ADFS instead of using the autoload from the federation metadata. This prevents Bookstack from automatically trying to use SLS. Here is the relevant .env for my ADFS SAML config:

## SAML Config
AUTH_METHOD=saml2
SAML2_NAME=ADFS
SAML2_EMAIL_ATTRIBUTE=mail
SAML2_EXTERNAL_ID_ATTRIBUTE=http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn
SAML2_DISPLAY_NAME_ATTRIBUTES=displayName
## Important - do not set set do the FederationMetadata URL
SAML2_IDP_ENTITYID=http://sts.example.com/adfs/services/trust
## Important
SAML2_AUTOLOAD_METADATA=false
SAML2_IDP_SSO=https://sts.example.com/adfs/ls/
## Important, make sure the following is commented out
#SAML2_IDP_SLO=null
## This is just your signing certificate on ADFS
SAML2_IDP_x509="MIIC2...."

I still have intentions of one day finishing my other patch, because SLS is cool, but it is not a necessity for me at the moment.

dani commented 3 years ago

I do have a similar issue with Lemonldap::NG as SAML IDP. SLO can't work because the the NameID isn't correct. Lemonldap::NG can't make sense of the request so respond with a code 400

coudot commented 3 years ago

The issue is in this part of the code: https://github.com/BookStackApp/BookStack/blob/65ddd16532e0329cc862f069064181593ad83253/app/Auth/Access/Saml2Service.php#L60

The logout method should be called with correct arguments, see https://github.com/onelogin/php-saml#initiate-slo

At least the correct NameID and SessionIndex that should be registered in user session when he authenticates.

theodor-franke commented 3 years ago

Is there a solution?

theodor-franke commented 3 years ago

2902 I created a PR that should resolve this Issue

ssddanbrown commented 2 years ago

As per #2902 a range of changes have now been made for BookStack v21.10.

The new SAML2_SP_x509 and SAML2_SP_x509_KEY options, which enable SP SLS signing, can be seen in the updated documentation: https://www.bookstackapp.com/docs/admin/saml2-auth/

Will therefore close this off but please open a new issue if there are problems with the updated implementation.

BookStackApp / BookStack

SAML SingleLogoutService Issues with ADFS #1925

2902 I created a PR that should resolve this Issue