enabling OAuth crashes Trino at startup

philicious commented 10 months ago

I wanted to enable OAuth but Trino crashes on startup without any helpful error. below is my chart config aswell as the stacktrace I get. enabling debug logs didnt show anything different.

Stacktrace mentions 3 errors but doesnt show them 🤯

When removing the server authentication type Trino starts up !

I tried with 436 (latest greatest) and 432 (latest chart default)

...
      - server:
          config:
            authenticationType: oauth2
      - additionalConfigProperties:
          - http-server.authentication.oauth2.issuer=https://foo.bar
          - http-server.authentication.oauth2.auth-url=https://foo.bar/oauth/authorize
          - http-server.authentication.oauth2.token-url=https://foo.bar/oauth/token
          - http-server.authentication.oauth2.jwks-url=https://foo.bar/oauth/discovery/keys
          - http-server.authentication.oauth2.userinfo-url=https://foo.bar/oauth/userinfo
          - http-server.authentication.oauth2.oidc.discovery=false
          - http-server.authentication.oauth2.client-id=42deadbeef
          - http-server.authentication.oauth2.client-secret=1337cafebabe
          - web-ui.authentication.type=oauth2

...
2024-01-22T17:45:54.343Z    INFO    main    Bootstrap    transaction.max-finishing-concurrency                                                   1                                                                          1                                   │
│     at ProvisionListenerStackCallback$Provision.provision(ProvisionListenerStackCallback.java:117)                                                                                                                                                              │
│     at ProvisionListenerStackCallback.provision(ProvisionListenerStackCallback.java:66)                                                                                                                                                                         │
│     at InternalProviderInstanceBindingImpl$CyclicFactory.get(InternalProviderInstanceBindingImpl.java:164)                                                                                                                                                      │
│     at ProviderToInternalFactoryAdapter.get(ProviderToInternalFactoryAdapter.java:40)                                                                                                                                                                           │
│     at SingletonScope$1.get(SingletonScope.java:169)                                                                                                                                                                                                            │
│     at InternalFactoryToProviderAdapter.get(InternalFactoryToProviderAdapter.java:45)                                                                                                                                                                           │
│     at FactoryProxy.get(FactoryProxy.java:60)                                                                                                                                                                                                                   │
│     at ProviderToInternalFactoryAdapter.get(ProviderToInternalFactoryAdapter.java:40)                                                                                                                                                                           │
│     at SingletonScope$1.get(SingletonScope.java:169)                                                                                                                                                                                                            │
│     at InternalFactoryToProviderAdapter.get(InternalFactoryToProviderAdapter.java:45)                                                                                                                                                                           │
│     at SingleParameterInjector.inject(SingleParameterInjector.java:40)                                                                                                                                                                                          │
│     at SingleParameterInjector.getAll(SingleParameterInjector.java:60)                                                                                                                                                                                          │
│     at SingleMethodInjector.inject(SingleMethodInjector.java:84)                                                                                                                                                                                                │
│     at MembersInjectorImpl.injectMembers(MembersInjectorImpl.java:146)                                                                                                                                                                                          │
│     at MembersInjectorImpl.injectAndNotify(MembersInjectorImpl.java:101)                                                                                                                                                                                        │
│     at Initializer$InjectableReference.get(Initializer.java:256)                                                                                                                                                                                                │
│     at Initializer.injectAll(Initializer.java:153)                                                                                                                                                                                                              │
│     at InternalInjectorCreator.injectDynamically(InternalInjectorCreator.java:180)                                                                                                                                                                              │
│     at InternalInjectorCreator.build(InternalInjectorCreator.java:113)                                                                                                                                                                                          │
│     at Guice.createInjector(Guice.java:87)                                                                                                                                                                                                                      │
│     at Bootstrap.initialize(Bootstrap.java:268)                                                                                                                                                                                                                 │
│     at Server.doStart(Server.java:135)                                                                                                                                                                                                                          │
│     at Server.lambda$start$0(Server.java:91)                                                                                                                                                                                                                    │
│     at io.trino.$gen.Trino_432____20240122_174552_1.run(Unknown Source)                                                                                                                                                                                         │
│     at Server.start(Server.java:91)                                                                                                                                                                                                                             │
│     at TrinoServer.main(TrinoServer.java:38)
|
│ 3 errors

mosabua commented 10 months ago

What was the problem?

philicious commented 10 months ago

@mosabua enabling OAuth also on the server while not having TLS setup causes the crash. The error-handling could be more graceful and informative in that case. if its only enabled on the UI, you dont need TLS setup if you are behind a LB that terminates TLS

mosabua commented 10 months ago

Well... if you do that things are essentially insecure inside the network/behind the lb. Trino requires TLS when any authentication is enabled.

philicious commented 10 months ago

absolutely. I was mislead by the doc which could be more clear about the possible different scenarios and their required config. I only needed to secure the UI, which had TLS terminated by LB already.

the OAuth doc assumes you want to enable both as I now understand.

furthermore, you have to lookup how to translate the config for the Helm Chart

philicious commented 10 months ago

and as I am looking at the docs again, I think the problem wasnt missing TLS, but rather missing internal-communication.shared-secret=<secret> , which isnt showing up in the OAuth example config

mosabua commented 10 months ago

Hm .. do you think it makes sense to add this everywhere in the code snippets even though it is clearly a required step in https://trino.io/docs/current/security/overview.html#suggested-configuration-workflow

and this sentence in the OAuth docs

Using TLS and a configured shared secret is required for OAuth 2.0 authentication.

philicious commented 10 months ago

Hm .. do you think it makes sense to add this everywhere in the code snippets even though it is clearly a required step in https://trino.io/docs/current/security/overview.html#suggested-configuration-workflow

well, it could help certain users. I never read that link until now tbh. here is my user-journey:

I first picked the Helm chart and as for most Helm charts, I assumed it had a good configuration. which it has!
I then quickly configured the extra catalogs I needed after Google lead me straight to https://trino.io/docs/current/connector/prometheus.html
afterwards, wanted to secure UI, similar to bunch of other tools before
Google and Trino-docs-Search lead me straight to the OAuth page, scroll ...yada yada, skip half of the text and copy the example config

and this sentence in the OAuth docs

Using TLS and a configured shared secret is required for OAuth 2.0 authentication.

the internal-secret doesnt seem to be required for UI-OAuth. at least its working for me w/o.

so to conclude, I think it would help to differentiate the OAuth doc for each of the two scenarios and if example config is shown, also include mandatory other config like internal-secret. even if it was already mentioned/linked in the description on same page. its probably also worth mentioning how to pass these configs via Helm as most(?) users will install Trino that way. or I am just living in a bubble where Helm is so common for everything in-cluster

mosabua commented 10 months ago

Thanks for your perspective @philicious .. the Helm chart definitely needs more work and consideration. I will see about updates to the regular docs as well.

philicious commented 10 months ago

sure thing. happy to help. ping me in here in case you want more feedback

deebify commented 10 months ago

@philicious how did you manage to enable UI through ingress with external authentication?

I have a "UI is disabled" which is expected when I use external authentication like OAuth.

philicious commented 10 months ago

@deebify its not true that "UI is disabled" when using OAuth. That error only is shown if you dont secure the UI. let me explain:

Working OAuth for UI:

      - additionalConfigProperties:
          - http-server.authentication.oauth2.issuer=https://foo.bar
          - http-server.authentication.oauth2.auth-url=https://foo.bar/oauth/authorize
          - http-server.authentication.oauth2.token-url=https://foo.bar/oauth/token
          - http-server.authentication.oauth2.jwks-url=https://foo.bar/oauth/discovery/keys
          - http-server.authentication.oauth2.userinfo-url=https://foo.bar/oauth/userinfo
          - http-server.authentication.oauth2.oidc.discovery=false
          - http-server.authentication.oauth2.client-id=42deadbeef
          - http-server.authentication.oauth2.client-secret=1337cafebabe
          - web-ui.authentication.type=oauth2
          - http-server.process-forwarded=true

http-server.process-forwarded=true this is required if you terminate TLS on LB or ingress-controller like nginx. https://trino.io/docs/current/security/tls.html#use-a-load-balancer-to-terminate-tls-httpsotherwise otherwise you get the "UI disabled". you dont need it if you terminate TLS in trino https://trino.io/docs/current/security/tls.html#secure-trino-directly

You cannot use OAuth with Trino if you only access it via HTTP

trinodb / charts

enabling OAuth crashes Trino at startup #125