Usage of PHP SensitiveParameter for the URI password component

[nyamsprod]

Q	A
Package	all.
Version	7.0

Question

The #[SensitiveParameter] PHP attribute was added to the $password property from URI. The problem is that it was only added on the class constructor and public user info modifier method. On master I have pushed a commit that improve the situation by applying the attribute everywhere the password component could be passed. This result in, for instance, having the attribute added to the UriString::parse.

On one hand this is consistent as we are hidden potential credential information but on the other hand this increase complexity for whoever expects to see the full URI when an error occurs. Also the use of password in URI is deprecated by RFC3986 so is the tradeoff worthwhile ?

@kelunik what do you think we should do ?

• Should we keep the SensitiveParameter "everywhere" like currently implemented on the master ?
• Should we remove it everywhere - not worth the complexity addition ?
• Should we revert to the initial usage ?

Thanks in advance for the commet/suggestion

[nyamsprod]

After some thoughts ... we could

Remove the SensitiveParaneter attribute all together and replace it with a toRedactedString which would return the URI with the password information masked.

Solution B seems to be favored by Guzzle and is less intrusive than correctly implementing the SensitiveParameter attribute.

However using the toRedactedString solution still requires:

• the developper to use the feature
• will not mask the password completely in the log like using the SensitiveParaneter attribute 🤔

[nyamsprod]

The possible API would be:

BaseUri::redactUserInfo():static : redact the password from user info
BaseUri::redactQueryPair(string ...$name):static : redact the value from the query string when the name is provided.

An alternative would be to have only one method:

BaseUri::redacSensitiveParameter(string ...$name):static

which would automatically redact the password from user info AND redact the value from the query string when the name is provided.

In both cases the value will be masked using 5 * characters.

for instance you would end up with:

echo BaseUri::from('https://toto:tata@example.com/?token=abcd1234&user_id=5678')
    ->redactUserInfo()
    ->redactQueryPair('token')
    ->getUriString();

or 

echo BaseUri::from('https://toto:tata@example.com/?token=abcd1234&user_id=5678')
    ->redacSensitiveParameter('token')
    ->getUriString();

// display https://toto:*****@example.com/?token=*****&user_id=5678

Of note:

The changes (new API) would land on the BaseUri class and not the Uri class which I want to stay small and finite. To not clutter if with millions of method which are all syntactic sugar methods and also allow for less coupling in case This incoming RFC where to pass.

The Uri class would only see the removal of the SensitiveParameter attribute.

[kelunik]

Hi @nyamsprod,

sorry I totally forgot to answer here, read it originally on my phone and wasn't keen on writing a longer text there.

On one hand this is consistent as we are hidden potential credential information but on the other hand this increase complexity for whoever expects to see the full URI when an error occurs. Also the use of password in URI is deprecated by RFC3986 so is the tradeoff worthwhile ?

String parameters are truncated in stack traces anyway if there're too long, no? If so, the expectation of seeing the full URI isn't one that happens in practise. User info is indeed deprecated for good reason, because it's easy to leak the secret authentication data as can be seen in this issue.

However, there's also another case to consider: What about capability URLs? The "Risk of Exposure" summarizes very well that keeping the really secret is challenging.

I think adding it to more rather than less places doesn't really hurt, but I'd focus on methods that pass passwords (including parse).

I wouldn't build redaction into BaseUri, but likely keep it a separate concern.

[nyamsprod]

@kelunik

Maybe we can have something that ressemble the UriTemplate class.

UriRedactor::__construct(string $mask = '****', string ...$defaultRedactedNames);
UriRedactor::redact(Stringable|string $uri, string ...$names): string

• The class can be instantiated with default query to be redacted
• And the redact method can add extra names to be redacted on a per uri case.

Of note, the redact returns a string and not an object because as I stated the returned value
may well become an invalid URI (depending on the mask chosen).

And we leave the BaseUri class free of that feature. Which is fine by me.

[kelunik]

Sounds good! Elastic has a list we can probably start with: https://www.elastic.co/guide/en/apm/agent/java/current/config-core.html#config-sanitize-field-names

[nyamsprod]

I reverted the added usage of SensitiveParameter as discussed.