[JENKINS-73499] Add a warning if there is a risk of exposing credentials through a non-TLS proxy connection

[jmdesprez]

See JENKINS-73499.

Testing done

Here is a screenshot of my local testing:

For now there is no automated testing. Let me know if I need to add one (I will need some guidance).

Proposed changelog entries

• Add a warning if there is a risk of exposing credentials through a non-TLS proxy connection

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

Before the changes are marked as ready-for-merge:

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[Wadeck]

Just some non-blocker comments :-)

[mawinter69]

The warning is always shown as soon as username or password are not empty. Even when I configure a https proxy url or (as in the screenshot) no url is configured.
Ideally this warning is only shown when using a http url.

[jmdesprez]

The warning is always shown as soon as username or password are not empty. Even when I configure a https proxy url

My understanding is that it is not possible to configure an https proxy. When filling out the configuration, one only provides the host and the port number, like in the below screenshot:

And it is not possible to specify which protocol the server will use:

So I've come to the conclusion that the connection to the proxy is always via http (non-TLS).

But I may have missed something.

or (as in the screenshot) no url is configured. Ideally this warning is only shown when using a http url.

My example is a bit silly because If there is no URL configured, then the entire proxy configuration will be discarded anyway.
Typically, the first two fields are filled in first, then the username and password.

I can, of course, change this, but then I suppose the validation will be more complex to write because several fields will be involved.

[timja]

I don't know how common https proxies are in the wild but maybe support for that should be added to go with this?

I've seen this:
https://www.chromium.org/developers/design-documents/secure-web-proxy/

and

https://security.stackexchange.com/a/23584

[jmdesprez]

I don't know how common https proxies are in the wild but maybe support for that should be added to go with this?

I've seen this: chromium.org/developers/design-documents/secure-web-proxy

and

security.stackexchange.com/a/23584

Based on (non-exhaustive) search on the Internet, https proxies are indeed not so common. On the client side, in addition to those you have mentioned, curl supports https proxies (see daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl). For example, OKHttp doesn't support it (see square/okhttp#3787).

maybe support for that should be added to go with this?

Do you mean as part of this ticket or in a new ticket?

[jmdesprez]

Additionally, it seems that people usually don't worry about the http connection because the proxy is usually on a private network.

[timja]

Based on (non-exhaustive) search on the Internet, https proxies are indeed not so common. On the client side, in addition to those you have mentioned, curl supports https proxies (see daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl). For example, OKHttp doesn't support it (see square/okhttp#3787).

Sounds risky to support it then as okhttp is used in a number of places we wouldn't be able to support it fully.

Additionally, it seems that people usually don't worry about the http connection because the proxy is usually on a private network.

Yes and also its only the initial CONNECT request and not e.g. every request so they've got to capture that initial request.

[jmdesprez]

@timja
I resolved the conversations and I updated the branch.
Please let me know if anything is missing so it can be marked as ready for merge.

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!