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

Supports the parameter extracredentials #6

Closed
shawnzhu opened this issue Jan 20, 2020 · 11 comments · Fixed by #116
Closed

Supports the parameter extracredentials #6

shawnzhu opened this issue Jan 20, 2020 · 11 comments · Fixed by #116
Labels
enhancement New feature or request

Comments

@shawnzhu
Copy link
Member

It introduced a new security feature called extraCredentials since release 302:

https://prestosql.io/docs/current/release/release-302.html

Add support for client-provided extra credentials that can be utilized by connectors. (trinodb/trino#124)

I've seen this new parameter extraCredentials has been documented and implemented for the JDBC driver but haven't seen such thing in the Python SDK.

@findepi
Copy link
Member

findepi commented Feb 16, 2020

Currently one has to use workaround:

presto.dbapi.connect(...., http_headers={'X-Presto-Extra-Credential': 'test.token.foo=bar}

note: exact encoding of extra credentials is subject to improvement in trinodb/trino#2780

@zyxue

@RameshByndoor
Copy link
Member

@findepi I tried the same in JDBC as well. See keys getting overwritten by the last one.
presto.dbapi.connect(...., http_headers={'X-Presto-Extra-Credential': 'myuser=username','X-Presto-Extra-Credential': 'mypassword=password'}
Given above example i see setting multiple keys with same name will get overridden in python dict, I am not sure whether it's because of requests or inside this library.

@zyxue
Copy link

zyxue commented May 3, 2021

Have you tried X-Trino instead of X-Presto?

@RameshByndoor
Copy link
Member

@zyxue I don't think that's the issue. Headers are getting passed. If you look at above example closely unlike Oauth token we need to set 2 keys for JDBC to work and it's passed in same header key. I guess the dictionary in presto client or requests library it get;s overwritten by last header key.

@hashhar
Copy link
Member

hashhar commented May 6, 2021

This is a "bug" in python-requests. The HTTP spec allows repeating headers as long as values are different. e.g. Cache-Control is a notable header that works that way.

It also says that compliant servers (Trino is compliant) should also treat comma-separated values as equivalent.

i.e.

X-Presto-Extra-Credential': 'mypassword=password,somethingelse=awesome'

is equivalent to (ordering is important):

X-Presto-Extra-Credential': 'mypassword=password'
X-Presto-Extra-Credential': 'somethingelse=awesome'

So try using the comma-separated form when setting headers from Python.

@matt12eagles
Copy link

bump on this, anyway to set user and pass extra credentials from python?

@shawnzhu
Copy link
Member Author

shawnzhu commented Oct 7, 2021

@matt12eagles see #6 (comment)

you could pass the extra credentials via request header X-Trino-Extra-Credential and using comma separated key=value pairs as value (Thanks to @hashhar). I haven't tried it by myself but looking forward any result

@matt12eagles
Copy link

@shawnzhu not having much luck :( Trying to pass in the extra credentials via the sqlachemy connect_args. if you find anything, please let me know.

@shawnzhu
Copy link
Member Author

@matt12eagles it works for me:

import http.client as http_client
from sqlalchemy.engine import create_engine

# dumping http headers via DEBUG
http_client.HTTPConnection.debuglevel = 1

# pass access token via connect_args
engine = create_engine(
  'trino://<username>:<password>@<trino-hostname>:443/',
  connect_args={'http_headers': {'X-Trino-Extra-Credential': 'foo1=bar, foo2=bar'}},
)

with engine.connect() as conn:
    rs = conn.execute('SELECT * FROM system.runtime.nodes')
    for row in rs:
        print(row)

If dumping the request headers, you will see the extra_credential header are sent like:

X-Trino-Extra-Credential: foo1=bar, foo2=bar

If using urllib3._connections.HTTPHeaderDict to construct headers with multiple request headers with the same header name, it will combine them into a single header with values of comma+space separated:

from urllib3._collections import HTTPHeaderDict

request_headers = HTTPHeaderDict()
request_headers.add('X-Trino-Extra-Credential', 'foo1=bar')
request_headers.add('X-Trino-Extra-Credential', 'foo2=bar')

engine = create_engine(
  'trino://<username>:<password>@<trino-hostname>:443/',
  connect_args={'http_headers': request_headers},
)

@shawnzhu shawnzhu added the documentation Improvements or additions to documentation label Oct 12, 2021
@matt12eagles
Copy link

@shawnzhu hands down the best response I have ever seen on git. Thank you!

Here are my logs, i'm doing this from the superset python connection... so very very close to the python cli yet still a bit different:

printing out new connection uri
trino://HIDDENUSER:HIDDENPASS@trinohost.apps.ocpnp.cotv.com:443/dcs_sqlserver?verify=/app/superset/apps-ocpnp-cotiviti-com-chain.pem

printing out params sent into connect class
{'poolclass': <class 'sqlalchemy.pool.impl.NullPool'>, 'connect_args': {'user':
'appuser'}}
printing out params after edit
{'poolclass': <class 'sqlalchemy.pool.impl.NullPool'>, 'connect_args': "{'user':
'appuser','X-Trino-Extra-Credential' :{'password':'hiddenpass',user:'hiddenuser'}}
"}

After your response, i'm going to replace 'X-Trino-Extra-Credential' with http-headers and see if it takes! Thank you!

@matt12eagles
Copy link

ANDDD @shawnzhu you got the key there!! Need http-headers!!! THANK YOU SO MUCH!! Beyond excited honestly!

@shawnzhu shawnzhu added enhancement New feature or request and removed documentation Improvements or additions to documentation labels Oct 13, 2021
@ebyhr ebyhr closed this as completed in #116 Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

6 participants