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

OIDC issuer behind a proxy cannot be accessed #1363

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tristanrobert
Copy link
Contributor

@tristanrobert tristanrobert commented Jan 6, 2025

Fixes #942

Context

Self hosted instance behind corporate proxy with OIDC issuer outside corporate LAN.

Proposed solution

Add ./ProxyAgent agent in openid-client custom http_options if issuer is on Internet

Related issues

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

Screenshots / Screencasts

@fflorent fflorent self-requested a review January 6, 2025 13:41
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patch @tristanrobert!

Some tests are failling. Some are just because of instabilities like Importer2, we can ignore them.
But some of them are due to the enhancement you introduced (these tests). Are you comfortable with adapting them? Or do you want me to do that for you?

The tests to adapt are here:
https://github.com/gristlabs/grist-core/blob/main/test/server/lib/OIDCConfig.ts

To setup the environment and to run the tests: https://github.com/gristlabs/grist-core/blob/main/documentation/develop.md

@tristanrobert
Copy link
Contributor Author

tristanrobert commented Jan 6, 2025

Thank you for your patch @tristanrobert!

Some tests are failling. Some are just because of instabilities like Importer2, we can ignore them. But some of them are due to the enhancement you introduced (these tests). Are you comfortable with adapting them? Or do you want me to do that for you?

The tests to adapt are here: https://github.com/gristlabs/grist-core/blob/main/test/server/lib/OIDCConfig.ts

To setup the environment and to run the tests: https://github.com/gristlabs/grist-core/blob/main/documentation/develop.md

I have fixed the tests @fflorent

@fflorent fflorent self-requested a review January 6, 2025 16:25
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds almost good.

I left you some remarks. Also I need to setup an environment to test your work. I am doing that ASAP.

app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
@tristanrobert
Copy link
Contributor Author

tristanrobert commented Jan 8, 2025

As we previously discussed @fflorent, I added the proxy agent tests in OIDCConfig test. On the other hand, a frontend test (code which I have not touched) is failing. I think recent changes are in error and need to be fixed

app/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
test/server/lib/OIDCConfig.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @tristanrobert!

@fflorent fflorent requested a review from dsagal January 10, 2025 09:14
@dsagal
Copy link
Member

dsagal commented Jan 14, 2025

The code here looks fine for doing what it intends, but I am not sure we want GRIST_HTTPS_PROXY to be used for OIDC automatically without an explicit change in configuration by the user.

GRIST_HTTPS_PROXY is used for a couple of things, notably webhooks, in particular because those could be made to URLs specified by the user of a document, and out of the control of the administrator. In other words, the proxy is used for untrusted requests. For OIDC, I can imagine wanting a separate proxy since the use-case is different. For example, for OIDC requests security may be more important, and more important to keep the proxy itself up-to-date. I can also imagine one might want to enable the proxy for OIDC because it's needed for logins, without having to use that proxy (or any proxy) for untrusted requests.

@paulfitz , what do you think?

@paulfitz
Copy link
Member

That's a good point @dsagal. There are two distinct usages here, and they could conflict. A proxy set up specifically for untrusted requests might not even be able to reach some internal OIDC-related server.

Not sure I have a good idea how to deal with this. Perhaps the proxying of untrusted traffic should have distinct configuration? The standard environment variable for this in other tools is https_proxy as opposed to our GRIST_HTTPS_PROXY so perhaps we could restrict the later to the "special" untrusted traffic and respect https_proxy more generally? Would that be too confusing? There was already a PR to use the proxy for fetching custom widget manifest that might need revisiting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs feedback
Development

Successfully merging this pull request may close these issues.

OIDC issuer behind a proxy cannot be accessed
4 participants