scylladb / scylla-cqlsh

A fork of the cqlsh code
Apache License 2.0
11 stars 29 forks source link

Error reading data from credential file #73

Closed slavavrn closed 3 months ago

slavavrn commented 4 months ago

In the cqlsh.py there is code that checks where the login and password are read from:

    username_from_cqlshrc = option_with_default(configs.get, 'authentication', 'username')
    password_from_cqlshrc = option_with_default(rawconfigs.get, 'authentication', 'password')
    if username_from_cqlshrc or password_from_cqlshrc:
        if password_from_cqlshrc and not is_file_secure(os.path.expanduser(CONFIG_FILE)):
            print("\nWarning: Password is found in an insecure cqlshrc file. The file is owned or readable by other users on the system.",
                  end='', file=sys.stderr)
        print("\nNotice: Credentials in the cqlshrc file is deprecated and will be ignored in the future."
              "\nPlease use a credentials file to specify the username and password.\n", file=sys.stderr)

If they are read from cqlshrc, a notification is issued to stderr. However, further in the code, the data from the file ~/.cassandra/credentials (or specified in the --credentials parameter) is ignored:

    if not options.username:
        credentials = configparser.ConfigParser()
        credentials.read(options.credentials)

        # use the username from credentials file but fallback to cqlshrc if username is absent from the command line parameters
        options.username = username_from_cqlshrc

    if not options.password:
        rawcredentials = configparser.RawConfigParser()
        rawcredentials.read(options.credentials)

        # handling password in the same way as username, priority cli > credentials > cqlshrc
        options.password = option_with_default(rawcredentials.get, 'plain_text_auth', 'password', password_from_cqlshrc)
        options.password = password_from_cqlshrc
    elif not options.insecure_password_without_warning:
        print("\nWarning: Using a password on the command line interface can be insecure."
              "\nRecommendation: use the credentials file to securely provide the password.\n", file=sys.stderr)

It is necessary to read data from credentials file like this:

    if not options.username:
        credentials = configparser.ConfigParser()
        credentials.read(options.credentials)

        # use the username from credentials file but fallback to cqlshrc if username is absent from the command line parameters
        options.username = option_with_default(credentials.get, 'plain_text_auth', 'username', username_from_cqlshrc)

    if not options.password:
        rawcredentials = configparser.RawConfigParser()
        rawcredentials.read(options.credentials)

        # handling password in the same way as username, priority cli > credentials > cqlshrc
        options.password = option_with_default(rawcredentials.get, 'plain_text_auth', 'password', password_from_cqlshrc)
    elif not options.insecure_password_without_warning:
        print("\nWarning: Using a password on the command line interface can be insecure."
              "\nRecommendation: use the credentials file to securely provide the password.\n", file=sys.stderr)

In fact, the notification sent to stderr is also erroneous

Link to the problematic code section: https://github.com/scylladb/scylla-cqlsh/blob/9d198006aa3dcb8d2afeb391479fdc28b7eaee5c/bin/cqlsh.py#L2491C1-L2507C113

mykaul commented 4 months ago

@fruch - thoughts?

fruch commented 4 months ago

From a quick look

That seems broken in

https://github.com/scylladb/scylla-cqlsh/commit/3230389058ba8cd467461e39966b8ef4dfe4e3a1

There is no real test coverage for that specific file, not sure why 2 configuration files are supported to begin with.

Anyhow @slavavrn you think you can issue a PR with the fix you suggested ?

We should also report it upstream (I'm guessing it's still broken there as well)

slavavrn commented 4 months ago

Based on the commit comment, 2 config files are supported because https://issues.apache.org/jira/browse/CASSANDRA-16983

But, in my opinion, it is also need to be fixed the notify output to stderr - the notify should be sent to stdout:

        print("\nNotice: Credentials in the cqlshrc file is deprecated and will be ignored in the future."
              "\nPlease use a credentials file to specify the username and password.\n", file=sys.stderr)

Yes I can issue a PR