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

Allow insecure https reqeusts #7596

Closed
3 tasks done
BitTheByte opened this issue Nov 19, 2022 · 10 comments
Closed
3 tasks done

Allow insecure https reqeusts #7596

BitTheByte opened this issue Nov 19, 2022 · 10 comments
Labels
enhancement An improvement of an existing feature security Related to application or infrastructure security

Comments

@BitTheByte
Copy link
Contributor

First check

  • I added a descriptive title to this issue.
  • I used the GitHub search to find a similar request and didn't find it.
  • I searched the Prefect documentation for this feature.

Prefect Version

2.x

Describe the current behavior

Not Implemented

Describe the proposed behavior

opt-in flag to allow sending insecure https requests e.g. PREFECT_CLIENT_ALLOW_INSECURE

Example Use

No response

Additional context

No response

@BitTheByte BitTheByte added enhancement An improvement of an existing feature status:triage labels Nov 19, 2022
@bunchesofdonald
Copy link
Contributor

Thanks for the idea @BitTheByte. Do you have a specific use case for this?

@BitTheByte
Copy link
Contributor Author

BitTheByte commented Nov 22, 2022

@bunchesofdonald using prefect with behind a proxy with a self-signed SSL certificate

@dhirschfeld
Copy link
Contributor

I would also like this option. Obviously in production I would have properly configured SSL certs but currently in DEV I've got self-signed certs and so without this option I can't test prefect :/

@dhirschfeld
Copy link
Contributor

I "fixed" the problem by patching httpx_settings.setdefault("verify", False) into the OrionClient:

def __init__(
self,
api: Union[str, FastAPI],
*,
api_key: str = None,
api_version: str = ORION_API_VERSION,
httpx_settings: dict = None,
) -> None:
httpx_settings = httpx_settings.copy() if httpx_settings else {}
httpx_settings.setdefault("headers", {})
if api_version:
httpx_settings["headers"].setdefault("X-PREFECT-API-VERSION", api_version)
if api_key:
httpx_settings["headers"].setdefault("Authorization", f"Bearer {api_key}")

It would be good to have a config setting to enable this without having to resort to patching the source.

@zanieb
Copy link
Contributor

zanieb commented Dec 9, 2022

Documentation for that option: https://www.python-httpx.org/advanced/#changing-the-verification-defaults

I am open to adding a PREFECT_API_SSL_VERIFY setting, but want to check with the team first.

cc @jawnsy

@jawnsy jawnsy added the security Related to application or infrastructure security label Dec 9, 2022
@jawnsy
Copy link
Contributor

jawnsy commented Dec 9, 2022

I'd propose a name like PREFECT_API_TLS_INSECURE_SKIP_VERIFY so that it's very obvious to people what this entails.

I think it's safer to add certificates to the CA store or tunnel TLS traffic through an unencrypted proxy (e.g. connect to a proxy over HTTP, use HTTP CONNECT to open an encrypted end-to-end tunnel to Prefect Cloud) rather than turning off verification. httpx describes this as the tunnelling method of proxying, instead of forwarding. I think that environment variables for this configuration should look something like: https_proxy=http://proxy-server

I'm not sure how to configure the trust store with httpx, as it seems to rely on one distributed by certifi, rather than the one included with the ca-certificates package, so turning off verification might be the easiest approach without changes to our agent. We could add a PREFECT_CACERT variable to allow that to be configured, perhaps.

Turning off TLS certificate verification entirely may be suitable in certain threat models, such as environments where you trust the network and can prevent DNS cache poisoning attacks, or for test purposes.

cc @loljawn

@zanieb
Copy link
Contributor

zanieb commented Dec 10, 2022

@jawnsy I was proposing a more generic name because the verify option can be passed a path to a CA bundle. In this case, we'd just pass through the bool or string to httpx. I believe this address your point at

I'm not sure how to configure the trust store with httpx...

If we want to be more explicit than the httpx options, we could do separate settings such as PREFECT_API_TLS_INSECURE_SKIP_VERIFY and PREFECT_API_TLS_CA_CERT_PATH.

@zanieb zanieb added needs:design Blocked by a need for an implementation outline needs:engineering feedback and removed status:triage labels Dec 10, 2022
@dhirschfeld
Copy link
Contributor

The self-signed certificates I have are just junk/throwaway certificates, automatically created for our ingress controller when we spin up our DEV cluster. As they're not long lived I'd rather not add them to my system trust store so I'd really just like an option to disable SSL verification altogether (as I've done by patching the source). I'm aware of the risks and on a secure network and just using it for testing.

I'm fine with a big scary name, and would even be ok with an annoying warning.

If the issue were just about getting prefect to use a self-signed certificate in my system cert bundle I believe that can be done by setting the SSL_CERT_FILE.

@zanieb
Copy link
Contributor

zanieb commented Dec 10, 2022

Ah great, that works for me as further justification for a dedicated PREFECT_API_TLS_INSECURE_SKIP_VERIFY setting. Feel free to open a pull request, otherwise it'll be on our backlog :)

(Note: We may need confirmation from our security team as well)

@zanieb zanieb added status:accepted and removed needs:design Blocked by a need for an implementation outline needs:engineering feedback labels Dec 10, 2022
@zanieb
Copy link
Contributor

zanieb commented Mar 30, 2023

Closed by #7850

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature security Related to application or infrastructure security
Projects
None yet
Development

No branches or pull requests

5 participants