Wrong request binding #104

gabbydotCMS · 2018-06-19T21:35:56Z

The previous version of the plugin used the HTTP-Redirect request binding, but we seem to be back to using HTTP-Artifact instead.

Unlike the response binding (protocol.binding), we don't have a property to override this binding, so we need to make sure is the HTTP-Redirect instead of request. ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact"

gabbydotCMS · 2018-06-19T21:54:51Z

@thstave

This is part of what we are missing in the new version of the plugin:

https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/3.0-4.2.x/src/com/dotcms/plugin/saml/v3/BindingType.java

I don't see that class anywhere in the new plugin. I'm checking how it's being used while generating the request to the IdP.

gabbydotCMS · 2018-06-19T22:25:58Z

If I'm not wrong ....

This is where the authentication request gets built:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/util/SamlUtils.java

Line 204 in d0b1865

    
           public static AuthnRequest buildAuthnRequest(final HttpServletRequest request, final IdpConfig idpConfig) {

That makes a call to getIPDSSODestination(idpConfig) here:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/util/SamlUtils.java

Line 205 in d0b1865

final String ipDSSODestination = getIPDSSODestination(idpConfig);

and that method is defined here:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/util/SamlUtils.java

Line 251 in d0b1865

public static String getIPDSSODestination(final IdpConfig idpConfig) {

which is different from its old definition:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/SamlUtils.java

Line 239 in 6756ff7

public static String getIPDSSODestination(final Configuration configuration) {

That previous version of the plugin, made the following call to define the request binding method:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/config/DefaultDotCMSConfiguration.java

Lines 142 to 143 in 6756ff7

    
           final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, 
        
                   BindingType.REDIRECT.getBinding());

which ended up using the binding type defined here:

https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/3.0-4.2.x/src/com/dotcms/plugin/saml/v3/BindingType.java

All that was substituted by

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/util/SamlUtils.java

Lines 267 to 268 in d0b1865

    
           final String redirectIdentityProviderDestinationSLOURL = MetaDataHelper 
        
           		.getIdentityProviderDestinationSLOURL(idpConfig);

which only uses the property DOTCMS_SAML_BINDING_TYPE.

I hope this helps.

Gabby

gabbydotCMS · 2018-06-20T18:49:01Z

[20/06/18 14:47:14:817 EDT]  INFO meta.DefaultMetaDescriptorServiceImpl: Parsing the Id Provider, with the entityId: urn:saml2:r3Jq54VOGp5mKC6:telus
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SSO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST(https://sso.connect.pingidentity.com/sso/idp/SSO.saml2?idpid=c57d9b5e-13d4-490b-ad68-95a19f65f9cd)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SSO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect(https://sso.connect.pingidentity.com/sso/idp/SSO.saml2?idpid=c57d9b5e-13d4-490b-ad68-95a19f65f9cd)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SLO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect(https://sso.connect.pingidentity.com/sso/SLO.saml2)
[20/06/18 14:47:14:819 EDT] DEBUG meta.DefaultMetaDescriptorServiceImpl: Add SLO binding urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST(https://sso.connect.pingidentity.com/sso/SLO.saml2)
[20/06/18 14:47:15:084 EDT] DEBUG parameters.DotsamlPropertiesService: Found protocol.binding : urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact

Changed by Gabby while com.dotcms.plugin.saml.v3.BindingType.java is introduced back in the latest code

gabbydotCMS · 2018-06-20T21:26:13Z

@thstave ... I changed the default value of the property dotcmsSamlProtocolBinding while you can work on adding back the enumeration that contained all the possible values:

https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/3.0-4.2.x/src/com/dotcms/plugin/saml/v3/BindingType.java

and its usage in the latest code similar to what we had in

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/config/DefaultDotCMSConfiguration.java

Lines 142 to 143 in 6756ff7

    
           final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, 
        
                   BindingType.REDIRECT.getBinding());

Please let me know if you have any doubts.

Gabby

Changed by Gabby while com.dotcms.plugin.saml.v3.BindingType.java is introduced back in the latest code

thstave · 2018-07-30T15:55:08Z

@gabbydotCMS ... Let me start with the reader's digest version and then I will answer some of your specific concerns.

In my testing (and I just re-ran them), the system does default to using HTTP-Redirect. So I need to ask if this occurs with all IDPs? You mention a parameter change you made, can you please relay what parameter change you made?

Now I'll try and address the specific concerns you mention.

BindingType class missing. The BindingType class was relocated and is now in a different package. You can find it here:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/key/BindingType.java

Lines 8 to 25 in d0b1865

    
           public enum BindingType 
        
           { 
        
           	AUTHN_REQUEST( "urn:mace:shibboleth:1.0:profiles:AuthnRequest" ), 
        
           	POST( "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" ), 
        
           	REDIRECT( "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" ); 
        
           	private final String binding; 
        
           	private BindingType( final String value ) 
        
           	{ 
        
           		this.binding = value; 
        
           	} 
        
           	public String getBinding() 
        
           	{ 
        
           		return binding; 
        
           	} 
        
           }

The property to override this parameter is "bindingtype" you can find it at:
https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlPropertyName.java#L38-L46

You reference differences in how metadata is accessed. As part of the system migrating away from using the old 'Configuration' class to store data to now using 'IdpConfig', 'MetaDataHelper' is now used to retrieve the default data. It will check both the default data as well as the specific IdpConfig. So seeing the code change in getIPDSSODestination from:

 final String redirectIdentityProviderDestinationSSOURL =                    configuration.getIdentityProviderDestinationSSOURL(configuration);

To using MetaDataHelper is a valid change.

final String redirectIdentityProviderDestinationSSOURL = MetaDataHelper
                .getIdentityProviderDestinationSSOURL(idpConfig);

Simarly, 'getIdentityProviderDestinationSSOURL' method you mentioned has also been moved. It is also part of 'MetaDataHelper'. The code you mention:

final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE,  BindingType.REDIRECT.getBinding());

is replaced by

String bindingType = DotsamlPropertiesService.getOptionString(idpConfig,
                DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE);

Which checks to determin if there is an override parameter and if not uses the default. The default value is defined at: https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlProperties.java#L16

I hope this answers your concerns. Please let me know what other information you need.

gabbydotCMS · 2018-07-31T02:49:50Z

@thstave,

This is the commit I made and the reason why now HTTP-Redirect is the default method now: a42ef10 .

If you undo that change, you'll go back to HTTP-Artifact.

In the previous version of the code, this line made sure that the default value set on DotsamlProperties.java for the binding type got changed to BindingType.REDIRECT:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/config/DefaultDotCMSConfiguration.java

Lines 142 to 143 in 6756ff7

    
           final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, 
        
                   BindingType.REDIRECT.getBinding());

So even when the enumeration still exists, the value BindingType.REDIRECT it's not been used anymore in the method getIdentityProviderDestinationSSOURL:

Original:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/config/DefaultDotCMSConfiguration.java

Lines 142 to 143 in 6756ff7

    
           final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, 
        
                   BindingType.REDIRECT.getBinding());

Latest:

plugin-com.dotcms.dotsaml/src/com/dotcms/plugin/saml/v3/config/MetaDataHelper.java

Lines 89 to 90 in d0b1865

    
           String bindingType = DotsamlPropertiesService.getOptionString(idpConfig, 
        
           		DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE);

Defaulting to urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact gave us troubles with at least two of the IdPs we've been testing with: Shibboleth and ADFS.

Gabby

thstave · 2018-07-31T13:10:22Z

@gabbydotCMS ... Ahhh. I think I see what you're getting at.

In the previous version

final String bindingType = configuration.getStringProperty(DotSamlConstants.DOTCMS_SAML_BINDING_TYPE, 
         BindingType.REDIRECT.getBinding());

BindingType.REDIRECT.getBinding() was being used as a default value. If DotSamlConstants.DOTCMS_SAML_BINDING_TYPE is not found in the override parameters use BindingType.REDIRECT.getBinding().

In this version

String bindingType = DotsamlPropertiesService.getOptionString(idpConfig, 
 		DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE);

default values are already defined and do not need to be passed as part of the call. As you already discovered they are defined in DotsamlProperties class. The reason for this was to allow Site-specific properties to be read from disk without adding another layer of conditional statements throughout the code. Site-based properties are read from disk and simply replace the hardcoded defaults. A one-time event.

So the statement above is equivalent to the psuedo old code.

final String bindingType = configuration.getStringProperty(DotsamlPropertyName.DOTCMS_SAML_BINDING_TYPE, 
         {Default Value defined in DotsamlProperties});

To ensure 'BindingType.REDIRECT.getBinding()' is the default value instead of using

private String dotcmsSamlProtocolBinding = SAML2_REDIRECT_BINDING_URI;

in DotsamlProperties - we would need to use.

private String dotcmsSamlProtocolBinding = BindingType.REDIRECT.getBinding();

This would make the code equivalent. As a sample see : https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/ethode-sp-endpoint-acs-rework/src/com/dotcms/plugin/saml/v3/parameters/DotsamlProperties.java#L16

gabbydotCMS · 2018-08-02T18:37:36Z

@thstave .. not sure what the next step is on this one.

dotcmsSamlProtocolBinding is already set to BindingType.REDIRECT.getBinding() in the latest code that we are using to test:

https://github.com/dotCMS/plugin-com.dotcms.dotsaml/blob/4.0-4.3.x/src/com/dotcms/plugin/saml/v3/parameters/DotsamlProperties.java#L16

So if we are already using BindingType.REDIRECT as the default value for that property, I assume I can remove the temporary patch I committed (a42ef10), and we'll be using HTTP-Redirect instead of HTTP-Artifact?

thstave · 2018-08-02T19:12:10Z

@gabbydotCMS ... Your patch was the right way to go. However, I would change your patch from;

private String dotcmsSamlProtocolBinding = SAML2_REDIRECT_BINDING_URI;

To

private String dotcmsSamlProtocolBinding =  BindingType.REDIRECT.getBinding();

for consistency. Then you should be set to go.

gabbydotCMS added the bug label Jun 19, 2018

gabbydotCMS assigned thstave Jun 19, 2018

gabbydotCMS pushed a commit that referenced this issue Jun 20, 2018

Temporary patch for issue #104

a42ef10

Changed by Gabby while com.dotcms.plugin.saml.v3.BindingType.java is introduced back in the latest code

gabbydotCMS pushed a commit that referenced this issue Jun 20, 2018

Temporary patch for issue #104

15a6250

Changed by Gabby while com.dotcms.plugin.saml.v3.BindingType.java is introduced back in the latest code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrong request binding #104

Wrong request binding #104

gabbydotCMS commented Jun 19, 2018

gabbydotCMS commented Jun 19, 2018 •

edited

Loading

gabbydotCMS commented Jun 19, 2018

gabbydotCMS commented Jun 20, 2018

gabbydotCMS commented Jun 20, 2018

thstave commented Jul 30, 2018

gabbydotCMS commented Jul 31, 2018 •

edited

Loading

thstave commented Jul 31, 2018

gabbydotCMS commented Aug 2, 2018

thstave commented Aug 2, 2018

Wrong request binding #104

Wrong request binding #104

Comments

gabbydotCMS commented Jun 19, 2018

gabbydotCMS commented Jun 19, 2018 • edited Loading

gabbydotCMS commented Jun 19, 2018

gabbydotCMS commented Jun 20, 2018

gabbydotCMS commented Jun 20, 2018

thstave commented Jul 30, 2018

gabbydotCMS commented Jul 31, 2018 • edited Loading

thstave commented Jul 31, 2018

gabbydotCMS commented Aug 2, 2018

thstave commented Aug 2, 2018

gabbydotCMS commented Jun 19, 2018 •

edited

Loading

gabbydotCMS commented Jul 31, 2018 •

edited

Loading