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.

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

Add #[SensitiveParameter] attribute to sensitive parameters #1082

Merged

Conversation

slknijnenburg
Copy link
Contributor

Fixes #1081, adds #[SensitiveParameter] to all potentially sensitive parameters, including key material, certificates and passphrases.

I wondered if this would be a BC break, but it seems that that is not the case. Child classes/interface implementations can apparently omit the attribute without any problems.

@Slamdunk Slamdunk changed the title Add SensitiveParameter attribute to sensitive parameters Add #[SensitiveParameter] attribute to sensitive parameters Nov 7, 2024
@Slamdunk Slamdunk added the Bug label Nov 7, 2024
Copy link
Collaborator

@Slamdunk Slamdunk left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution.

As far as I know, #[SensitiveParameter] only makes sense in string parameters, please update the PR to only target:

  1. \Lcobucci\JWT\Signer\Key\InMemory methods
  2. \Lcobucci\JWT\Signer\OpenSSL::createSignature
  3. \Lcobucci\JWT\Signer\OpenSSL::verifySignature

Also, as this can be considered a Bug please target the 5.4.x branch.

@Slamdunk Slamdunk added this to the 5.4.2 milestone Nov 7, 2024
@slknijnenburg slknijnenburg changed the base branch from 5.5.x to 5.4.x November 7, 2024 11:25
src/Signer/Key/InMemory.php Show resolved Hide resolved
src/Signer/OpenSSL.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@Slamdunk Slamdunk left a comment

Choose a reason for hiding this comment

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

Awesome, few final fixes and we can 🚢 it 💪

src/Signer/Key/InMemory.php Show resolved Hide resolved
src/Signer/Key/InMemory.php Show resolved Hide resolved
src/Signer/OpenSSL.php Outdated Show resolved Hide resolved
Adds `#[SensitiveParameter]` to all potentially sensitive parameters,
including key material, certificates and passphrases.
@Slamdunk Slamdunk merged commit ea1ce71 into lcobucci:5.4.x Nov 7, 2024
21 checks passed
@TimWolla
Copy link

TimWolla commented Nov 7, 2024

As far as I know, #[SensitiveParameter] only makes sense in string parameters

This is false. The attribute is agnostic to the parameter type and the Exception handler of your choice might opt to also process objects (e.g. by printing their __debugInfo()) output. The attribute should be apply to anything that is sensitive, independent of type.

@Slamdunk
Copy link
Collaborator

Slamdunk commented Nov 7, 2024

Mmm, by this logic everything is sensitive, if my debug handler dumps the entire memory and filesystem to the logger.

I think handling the default behaviour is enough for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark sensitive parameters with #[SensitiveParameter]
4 participants