oras icon indicating copy to clipboard operation
oras copied to clipboard

Avoid requiring passing --plain-http when specifying --insecure

Open sajayantony opened this issue 1 year ago • 18 comments

What is the version of your ORAS CLI

1.0.0

What would you like to be added?

Currently if the users specifies --insecure the call fails if the server is not responding with https

oras version Version:    1.0.0+Homebrew Go version: go1.20.2 
oras login -u user--insecure remote-registry:5000 
Error: Get "https://remote-registry:5000/v2/": http: server gave HTTP response to HTTPS client

Users are expected to pass a --plain http as well in this case and we can follow precedence from docker for this to make the experience better - https://github.com/docker/cli/blob/de0d30ff24fad55df89adcb3c01d10bb1821c8db/cli/registry/client/client.go#L131

Why is this needed for ORAS?

Improve user experience.

Are you willing to submit PRs to contribute to this feature?

  • [ ] Yes, I am willing to implement it.

Reference: https://go-review.googlesource.com/c/go/+/382117/1..2/src/net/http/client.go#202

sajayantony avatar Apr 04 '23 01:04 sajayantony

Can we completely remove --plain-http? It is redundant if we implement the above.

toddysm avatar Apr 04 '23 17:04 toddysm

Can we completely remove --plain-http? It is redundant if we implement the above.

SGTM. --insecure can cover --plain-http with minor performance cost. Considering it's not expected to be used in production env, the overhead is acceptable.

  • try using HTTPS.
  • If HTTPS is available
    • if the certificate is invalid and --insecure is set, ignore the error about the certificate.
  • If HTTPS is not available and --insecure is set, fall back to HTTP.

qweeah avatar Apr 06 '23 06:04 qweeah

Can we completely remove --plain-http? It is redundant if we implement the above.

No, removing --plain-http is a breaking change to oras v1.

shizhMSFT avatar Apr 06 '23 06:04 shizhMSFT

I have an implementation but the failing e2e test helps me found a case that --insecure cannot be used to imply --plain-http: when --resolve is used to customize DNS for a HTTP registry service.

Taking oras discover as an example, below command is used to run the test spec:

$ oras discover oras.e2e.test/command/artifacts:foobar -o json --resolve oras.e2e.test:80:127.0.0.1:5000 --username hello -*** --insecure

The registry service is running at port 5000 in plain HTTP, and oras is trying to access it via the domain name oras.e2e.test. According to mentioned docker's implementation, the request will be sent to oras.e2e.test:443 first, which will not follow the provided DNS from oras.e2e.test:80 to 127.0.0.1:5000 and fails. The returned error will not suggest try HTTP rather than HTTPS so the whole process fails.

Of course, user can add another DNS rule via --resolve oras.e2e.test:443:127.0.0.1:5000 but it's much easier to set --plain-http.

qweeah avatar Apr 17 '23 13:04 qweeah

@qweeah I am confused why this is even a problem. 5000 is a non-standard (HTTP/S) port and can be used for both HTTP and HTTPS (as a matter of fact, every port can be used for both). Also, the port of the registry may be different than 5000. The standard fallback should always be 443 --> 80 as those are considered standard ports and there shouldn't be a fallback from 443 --> 5000. In this case I would say we should fix our test case and not add a switch to satisfy a single test case. Such mappings should be explicit.

toddysm avatar Apr 19 '23 12:04 toddysm

I am confused why this is even a problem. 5000 is a non-standard (HTTP/S) port and can be used for both HTTP and HTTPS (as a matter of fact, every port can be used for both)

It is common to use port 5000 when debugging local http/https services.

Also, the port of the registry may be different than 5000. The standard fallback should always be 443 --> 80 as those are considered standard ports and there shouldn't be a fallback from 443 --> 5000.

In the failed cases, both the HTTPS and HTTP request need to be sent to 5000 (not 443 --> 5000). Basically, this is a use case not covered by Docker CLI since Docker CLI doesn't support customized DNS rules.

In this case I would say we should fix our test case and not add a switch to satisfy a single test case. Such mappings should be explicit.

To summarize, Docker's fallback will fail in ORAS CLI when user uses --resolve with port redirection. Just need to confirm if @toddysm is suggesting make to this use case invalid.

qweeah avatar Apr 19 '23 15:04 qweeah

@qweeah Can we step back and define what is the experience we would like to achieve here. Writing code that will target a specific (hardcoded) port may result in strange behavior. What will happen if a user needs to run the local registry on 5050 instead? What is the experience they will encounter? I think the only special/automatic handling should be 443-->80. For every other port the logic should be (IMHO):

  • Check if there is cert
    • If there is cert, check if it trusted
      • If it is trusted use HTTPS
      • If it is not trusted, check for --insecure
        • If there is --insecure establish (untrusted HTTPS) connection
        • If there is NO --insecure break
    • If there is no cert, use HTTP on the same port

My point is that the fallback should be "secure" ("trusted") --> "unsecure" ("untrusted") and not from portA --> portB. Ports can be arbitrary.

toddysm avatar Apr 20 '23 10:04 toddysm

My point is that the fallback should be "secure" ("trusted") --> "unsecure" ("untrusted") and not from portA --> portB. Ports can be arbitrary.

What we are trying to do is https --> http auto handling via --insecure but that doesn't mean we are doing 443 --> 80. Let's look at the original case you raised: you are trying to login a plain-http zot registry running at port 5000, if --insecure is set, both https and https requests will be sent to port 5000.

What we are trying to do is to optimize the insecure user experience, changes are introduced only when --insecure is set. There is no change in "secure" ("trusted") mode.

The logic should be (IMHO):

  • check for --insecure
    • if it's not set, no change
    • if it's set, try https

qweeah avatar Apr 20 '23 11:04 qweeah

Docker pattern might fail DNS resolution when user uses another flag --resolve. I guess we have gap in what --resolve do and what port redirection is, we can fill the gap offline.

qweeah avatar Apr 20 '23 12:04 qweeah

Synchronized offline with @toddysm that using https-->http auto handling like below command will fail:

oras login oras.e2e.test \
  --resolve oras.e2e.test:80:0.0.0.0:5000  --insecure

ORAS user have below options to make it work

# 1. specify port in the registry
oras login oras.e2e.test:80 \
  --resolve oras.e2e.test:80:0.0.0.0:5000 \
  --insecure
# or
oras login oras.e2e.test:5000 \
  --resolve oras.e2e.test:5000:0.0.0.0 \
  --insecure

# 2. add a rule for 443 port redirection
oras login oras.e2e.test \
  --resolve oras.e2e.test:80:0.0.0.0:5000 \
  --resolve oras.e2e.test:443:0.0.0.0:5000 \
  --insecure

qweeah avatar Apr 25 '23 01:04 qweeah

@qweeah I think # 1. specify the port in the registry is the most common case. # 2. add a rule for 443 port redirection listed above is not necessary and looks cumbersome when using --resolve flag twice in a command. It would be much more friendly to tell users if they missed specifying a port number like in case 2.

FeynmanZhou avatar Apr 25 '23 09:04 FeynmanZhou

I found that if a port is specified in the registry name for login, then it will also be recorded in the auth config file, e.g. if I used below command to login to 0.0.0.0:5000 with customized domain name:

# login with port 
oras login oras.e2e.test:80 \
  --resolve oras.e2e.test:80:0.0.0.0:5000 \
  --insecure

Then the auth config file will be looked like,

{
...
        "auths": {
        ...
                "oras.e2e.test:80": {
                        "auth": "**************"
                }
        }
}

Accordingly, if user want to use this config file in other tools, they need to specify same port used in the oras login command, otherwise the stored credential cannot be matched and fetched from the auth config file.

# suppose `0.0.0.0 oras.e2e.test` is added to /etc/hosts 
$ docker pull oras.e2e.test:5000/hello:v1
Error response from daemon: Head "http://oras.e2e.test:5000/v2/hello/manifests/v1": no basic auth credentials

$ docker pull oras.e2e.test/hello:v1
Error response from daemon: Head "http://oras.e2e.test/v2/hello/manifests/v1": no basic auth credentials

qweeah avatar Apr 26 '23 14:04 qweeah

If a user want login a registry hosted at 0.0.0.0:5000, and the registry credential stored without port, they will have to use

oras login oras.e2e.test \
  --resolve oras.e2e.test:80:0.0.0.0:5000 \
  --resolve oras.e2e.test:443:0.0.0.0:5000 \
  --insecure

In this case the user experience is much complicated than oras login oras.e2e.test --plain-text --resolve oras.e2e.test:5000:0.0.0.0.

qweeah avatar Apr 26 '23 14:04 qweeah

Investigated implementation of existing registry clients tools and documented in a HackMD doc.

qweeah avatar May 08 '23 08:05 qweeah

Moving it to 2.0 since the auto fallback incurs security level change, which is breaking. If HTTPS is not supported and falls back to HTTP, the data transmission is not encrypted.

qweeah avatar May 10 '23 00:05 qweeah

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Jul 23 '23 01:07 github-actions[bot]

Should we consider not auto closing any issue that’s in a milestone?

sajayantony avatar Jul 24 '23 01:07 sajayantony

Should we consider not auto closing any issue that’s in a milestone?

Definitely, will raise a PR to add it.

qweeah avatar Jul 24 '23 03:07 qweeah