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

Document that SHA-1 must not be used #242

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@klausi
Contributor

klausi commented Jul 31, 2017

Follow-up to #237 . We cannot change SHA1 as default value in the 2.x version of this library, but we can at least warn new users to use the correct settings.

Show outdated Hide outdated settings_example.php Outdated
@@ -276,6 +276,25 @@ file, rename and edit it.
<?php
$settings = array (
// Security settings that must be set to avoid insecure SHA-1 usage.

This comment has been minimized.

@pitbulk

pitbulk Jul 31, 2017

Contributor

that block where signatureAlgorithm digestAlgorithm are described already appears down...

@pitbulk

pitbulk Jul 31, 2017

Contributor

that block where signatureAlgorithm digestAlgorithm are described already appears down...

This comment has been minimized.

@klausi

klausi Jul 31, 2017

Contributor

Right, but we need that block in the basic settings now. We need parts of the security options here, but the advanced options list all security options. So maybe that duplication is fine?

@klausi

klausi Jul 31, 2017

Contributor

Right, but we need that block in the basic settings now. We need parts of the security options here, but the advanced options list all security options. So maybe that duplication is fine?

This comment has been minimized.

@pitbulk

pitbulk Jul 31, 2017

Contributor

Basic settings only contain IdP and SP info, security stuff is on advanced_settings

If you want you can add a new section about the sha1 deprecation info on the README after
https://github.com/onelogin/php-saml#idp-with-multiple-certificates

Something like SHA-1 Deprecation, and explain it why is important

@pitbulk

pitbulk Jul 31, 2017

Contributor

Basic settings only contain IdP and SP info, security stuff is on advanced_settings

If you want you can add a new section about the sha1 deprecation info on the README after
https://github.com/onelogin/php-saml#idp-with-multiple-certificates

Something like SHA-1 Deprecation, and explain it why is important

This comment has been minimized.

@klausi

klausi Jul 31, 2017

Contributor

Good idea, there is already a security warning section about the strict parameter, added SHA1 info there.

Basic settings already contain baseurl and strict so I think it is fine to also include the signature and digest algorithm there?

@klausi

klausi Jul 31, 2017

Contributor

Good idea, there is already a security warning section about the strict parameter, added SHA1 info there.

Basic settings already contain baseurl and strict so I think it is fine to also include the signature and digest algorithm there?

// 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'
// 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha384'
// 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha512'
'signatureAlgorithm' => 'http://www.w3.org/2000/09/xmldsig#rsa-sha1',
// Insecure options that must not be used (still available for backwards compatibility):

This comment has been minimized.

@pitbulk

pitbulk Jul 31, 2017

Contributor

maybe add a reference why sha1 is deprecated and its use must be avoided?

@pitbulk

pitbulk Jul 31, 2017

Contributor

maybe add a reference why sha1 is deprecated and its use must be avoided?

This comment has been minimized.

@klausi

klausi Jul 31, 2017

Contributor

This is covered now in the security warnings section.

@klausi

klausi Jul 31, 2017

Contributor

This is covered now in the security warnings section.

@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk Aug 14, 2017

Contributor

Can we apply the suggested changes (remove algorithm info from basic settings files (including readme section) so I can merge?

Contributor

pitbulk commented Aug 14, 2017

Can we apply the suggested changes (remove algorithm info from basic settings files (including readme section) so I can merge?

@klausi

This comment has been minimized.

Show comment
Hide comment
@klausi

klausi Aug 17, 2017

Contributor

The problem is that the SHA-256 security settings are basic settings now, since you always need to set them to have a secure setup. If we remove it then people will not set it?

Contributor

klausi commented Aug 17, 2017

The problem is that the SHA-256 security settings are basic settings now, since you always need to set them to have a secure setup. If we remove it then people will not set it?

pitbulk added a commit that referenced this pull request Nov 6, 2017

@pitbulk pitbulk closed this Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment