Oauth2-server: "Ensure the server is the exclusive owner of the key" is wrong

[gemal]

the implementation is wrong
if ($keyPathPerms !== '600') {

you cant say that this is the perfect permission.
you might have set up the file with permission to the group

which you then have
'660'

[gemal]

I'm talking about this commit:
https://github.com/thephpleague/oauth2-server/commit/2f8de3d2302beb490abb9475cf426148801c25c4

how can I help in fixing this?

[alexbilbie]

Please feel free to submit a pull request

[LavaToaster]

@alexbilbie Is this supposed to be for _all_ oauth keys, or just the ones passed via a string and saved to /tmp

[libasoles]

@gemal Have you created that PR? If not, I think I will.

[andersonamuller]

Does anyone have a fix for this case?

• Container PHP-FPM runs on www-data;
• OAuth keychain are Kubernetes secrets mounted and owned by root;

AFAIK, you cannot change that last part.

See also https://github.com/laravel/passport/issues/418#issuecomment-312866308

[alexbilbie]

@andersonamuller could you copy the keys out of Kubernetes secrets mount into a temp directory when the container starts using an entrypoint script? You might need to use UID and GID to manage privileges

[andersonamuller]

USER defined in the container is already www-data so I cannot create a script to call a chown the user does not have permissions.

[gemal]

I still think just check for if ($keyPathPerms !== '600') {
is wrong

you could have a perfectly secure system without the permissions being 600

[JEDIBC]

An easy workaround is to use the values of the keys and not the path to the key files.

[JEDIBC]

The recent commit 80fc8e654b6ab6ba66000ddd7b95f8d7203c2443 won't change the problem for symfony users as errors are turned into exception by the framework.

Perhaps a modification of the constructor like below would allow more flexibility in the use of the keys :

/**
 * CryptKey constructor.
 *
 * @param string      $keyPath
 * @param null|string $passPhrase
 * @param bool        $checkFileMode
 * @param int         $fileMode
 */
public function __construct($keyPath, $passPhrase = null, $checkFileMode = true, $fileMode = 0600)
{
    if (preg_match(self::RSA_KEY_PATTERN, $keyPath)) {
        $keyPath = $this->saveKeyToFile($keyPath);
    }

    if (strpos($keyPath, 'file://') !== 0) {
        $keyPath = 'file://' . $keyPath;
    }

    if (!file_exists($keyPath) || !is_readable($keyPath)) {
        throw new \LogicException(sprintf('Key path "%s" does not exist or is not readable', $keyPath));
    }

    if ($checkFileMode) {
        // Verify the permissions of the key
        $keyPathPerms = decoct(fileperms($keyPath) & 0777);
        $stringFileMode = base_convert((string) $fileMode, 10, 8);
        if ($keyPathPerms !== $stringFileMode) {
            // Attempt to correct the permissions
            if (chmod($keyPath, $fileMode) === false) {
                // @codeCoverageIgnoreStart
                trigger_error(
                    sprintf(
                        'Key file "%s" permissions are not correct, should be %s instead of %s, unable to automatically resolve the issue',
                        $keyPath,
                        $stringFileMode,
                        $keyPathPerms
                    ),
                    E_USER_NOTICE
                );
                // @codeCoverageIgnoreEnd
            }
        }
    }

    $this->keyPath    = $keyPath;
    $this->passPhrase = $passPhrase;
}

[klemenb]

+1 for the issue with Kubernetes deployments (and other similar environments).

The main issue is that secrets are mounted as "read-only" volumes and the "chmod" function itself fails with chmod: changing permissions of X: Read-only file system.

I understand that this was done with best intentions, but being "too smart" will almost always break things for many valid use cases out there.

[alexbilbie]

I understand that this was done with best intentions, but being "too smart" will almost always break things for many valid use cases out there.

Am coming to the same conclusion quickly

[yannickl88]

I've made a PR #776 which proposes two things:

• Remove the code that chmods files but trigger a USER_NOTICE when not set to 600.
• Option to skip the check. (for dev or special cases)

I think this is a solution for this issue.

[AbhijeetSomwanshi]

@alexbilbie I am getting the following error on upgrade of Passport "laravel/passport": "^3.0" Please help its on live server and urgent. Any quick fix will be greatly appreciated.

chmod(/var/www/html/storage/oauth-private.key): Operation failed: Operation not permitted

Thanks

[alexbilbie]

This issue has been resolved by 6.0.2

[vibrantBits]

Is it fixed? I just made a fresh install in homestead and got:

Key file "file:///home/vagrant/Development/.../storage/oauth-public.key" permissions are not correct, should be 600 or 660 instead of 777

[klemenb]

Laravel catches the E_USER_NOTICE in its exception handler.

You can add error_reporting(E_ALL & ~E_USER_NOTICE); to one of your service providers to prevent this.

[jigarpt]

I am also getting the same error on windows 10 system.

Key file "file://E:/.../storage/oauth-public.key" permissions are not correct, should be 600 or 660 instead of 666