Azure / azure-cli-extensions

Public Repository for Extensions of Azure CLI.
https://docs.microsoft.com/en-us/cli/azure
MIT License
382 stars 1.21k forks source link

[aks-preview] Check for kube config doesn't properly handle symlinks #1525

Open lumaxis opened 4 years ago

lumaxis commented 4 years ago

Extension name (the extension in question)

aks-preview

Description of issue (in as much detail as possible)

The check for the file permissions on .kube/config don't properly account for the file being a symlink. Symlinks themselves don't have permissions so it would be important to follow the symlink if it exists and check for the actual file, otherwise this check fails erroneously: https://github.com/Azure/azure-cli-extensions/blob/9a809ebcada690b29ccacf59169f562fb6101a5d/src/aks-preview/azext_aks_preview/custom.py#L2532-L2535

yonzhan commented 4 years ago

aks-preview

ghost commented 4 years ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @Azure/aks-pm.

djsly commented 4 years ago

Correct, I was able to reproduce, this is a simple warning though and the az aks get-credentials command will still properly merge the configuration in you ~/.kube/config

az aks get-credentials

✖ az aks get-credentials -n <name> -g <name> --admin 
The behavior of this command has been altered by the following extension: aks-preview
/Users/<me>/.kube/config has permissions "755".
It should be readable and writable only by its owner.
Merged "<name>" as current context in /Users/<me>/.kube/config
❯ ls -la ~/.kube/
lrwxr-xr-x     1 <me>  <id>      38      29 May 22:55 config -> /Users/<me>/.kube/config.real
-rw-------     1 <me>  <id>  159152 29 May 22:57 config.real
djsly commented 4 years ago

a quick PR could look like this

    # check that ~/.kube/config is only read- and writable by its owner
    if platform.system() != 'Windows':
        final_existing_file = existing_file
        if os.path.islink(final_existing_file):
            final_existing_file = os.readlink(path)
        existing_file_perms = "{:o}".format(stat.S_IMODE(os.lstat(final_existing_file).st_mode))
        if not existing_file_perms.endswith('600'):
            logger.warning('%s has permissions "%s".\nIt should be readable and writable only by its owner.',
                           final_existing_file, existing_file_perms)