-
Notifications
You must be signed in to change notification settings - Fork 70
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
Support Basic authentication for devfile factory URL #451
Conversation
Signed-off-by: Igor Vinokur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinokurig could you please provide image for testing in the PR description?
...i-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/DefaultFactoryUrl.java
Outdated
Show resolved
Hide resolved
...pi-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/RemoteFactoryUrl.java
Outdated
Show resolved
Hide resolved
...ctory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/DefaultFactoryUrlTest.java
Outdated
Show resolved
Hide resolved
...i-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/DefaultFactoryUrl.java
Outdated
Show resolved
Hide resolved
updated the description |
...i-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/DefaultFactoryUrl.java
Outdated
Show resolved
Hide resolved
if (!isNullOrEmpty(username) || !isNullOrEmpty(password)) { | ||
return Optional.of( | ||
format( | ||
"%s:%s", | ||
isNullOrEmpty(username) ? "" : username, isNullOrEmpty(password) ? "" : password)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit lost, can we really handle a case from auth perspective if either username or password is null / blank? I thought we need both, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is probably for token:
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we don't need both parts. If we have only username, we pass the credentials as username:
. From the authentication prospective this case is considered as a token. I agree that it looks a bit weird, but it coms from the curl
behaviour:
curl https://<token>@dev.azure.com/vinokurig/test/_apis/git/repositories/test/items?path=%2Fdevfile.yaml -v
* Trying 185.199.109.133:443...
* Connected to raw.githubusercontent.com (185.199.109.133) port 443 (#0)
* ALPN: offers h2
* ALPN: offers http/1.1
* CAfile: /etc/ssl/cert.pem
* CApath: none
* (304) (OUT), TLS handshake, Client hello (1):
* (304) (IN), TLS handshake, Server hello (2):
* (304) (IN), TLS handshake, Unknown (8):
* (304) (IN), TLS handshake, Certificate (11):
* (304) (IN), TLS handshake, CERT verify (15):
* (304) (IN), TLS handshake, Finished (20):
* (304) (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / AEAD-AES128-GCM-SHA256
* ALPN: server accepted h2
* Server certificate:
* subject: C=US; ST=California; L=San Francisco; O=GitHub, Inc.; CN=*.github.io
* start date: Feb 21 00:00:00 2023 GMT
* expire date: Mar 20 23:59:59 2024 GMT
* subjectAltName: host "raw.githubusercontent.com" matched cert's "*.githubusercontent.com"
* issuer: C=US; O=DigiCert Inc; CN=DigiCert TLS RSA SHA256 2020 CA1
* SSL certificate verify ok.
* Using HTTP2, server supports multiplexing
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Server auth using Basic with user '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'
* h2h3 [:method: GET]
* h2h3 [:path: /vinokurig/private/master/devfile.yaml]
* h2h3 [:scheme: https]
* h2h3 [:authority: raw.githubusercontent.com]
* h2h3 [authorization: Basic <token plus : in Base 64> ]
So curl https://<token>@dev.azure.com/...
equals curl https://dev.azure.com/... -H 'Authorization: Basic <token + : in base64>'
public String fetchContent(String fileURL, String credentials) | ||
throws IOException, DevfileException { | ||
return fetchContent(fileURL, false, credentials); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use Optional here also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use Optional
or Nullable
here. This is an overloaded method which is supposed to receive non null parameters.
} | ||
|
||
protected String fetchContent(String fileURL, boolean skipAuthentication) | ||
private String fetchContent( | ||
String fileURL, boolean skipAuthentication, @Nullable String credentials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to use Optional here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that passing Optional
as a parameter is a good idea: https://stackoverflow.com/questions/31922866/why-should-java-8s-optional-not-be-used-in-arguments
This is a private method so I don't think it will affect something.
...ctory/src/test/java/org/eclipse/che/api/factory/server/urlfactory/DefaultFactoryUrlTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinokurig great job 👍 please consider a few suggestions (Optionals + more fancy URL tests coverage with paramters)
@vinokurig also would be nice if you can add some docs references about this cool feature 👍 |
…i/factory/server/urlfactory/DefaultFactoryUrlTest.java Co-authored-by: Ilya Buziuk <[email protected]>
Build 3.6 :: server_3.x/127: Console, Changes, Git Data |
Build 3.6 :: sync-to-downstream_3.x/2510: Console, Changes, Git Data |
Build 3.6 :: push-latest-container-to-quay_3.x/1815: Console, Changes, Git Data |
Build 3.6 :: update-digests_3.x/2297: Console, Changes, Git Data |
Build 3.6 :: push-latest-container-to-quay_3.x/1815: Copied: server-rhel8; /job/DS_CI/job/update-digests_3.x triggered; |
Build 3.6 :: get-sources-rhpkg-container-build_3.x/2370: server : 3.x :: Build 50958640 : quay.io/devspaces/server-rhel8:3.6-16 |
Build 3.6 :: copyIIBsToQuay/1041: Console, Changes, Git Data |
Build 3.6 :: operator-bundle_3.x/951: Console, Changes, Git Data |
Build 3.6 :: sync-to-downstream_3.x/2510: Build container: devspaces-server synced; /DS_CI/get-sources-rhpkg-container-build_3.x/2370 triggered; |
Build 3.6 :: server_3.x/127: Upstream sync done; /DS_CI/sync-to-downstream_3.x/2510 triggered |
Build 3.6 :: sync-to-downstream_3.x/2512: Console, Changes, Git Data |
Build 3.6 :: push-latest-container-to-quay_3.x/1817: Console, Changes, Git Data |
Build 3.6 :: push-latest-container-to-quay_3.x/1817: Copied: devspaces-operator-bundle; bundle-generated updated; |
Build 3.6 :: copyIIBsToQuay/1043: Console, Changes, Git Data |
Build 3.6 :: sync-to-downstream_3.x/2512: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/2372 triggered; /job/DS_CI/job/dsc_3.x triggered; |
Build 3.6 :: operator-bundle_3.x/951: Upstream sync done; /DS_CI/sync-to-downstream_3.x/2512 triggered |
Build 3.6 :: update-digests_3.x/2297: Detected new images: rebuild operator-bundle |
Build 3.6 :: dsc_3.x/663: Console, Changes, Git Data |
Build 3.6 :: dsc_3.x/663: 3.6.0 CI |
Build 3.6 :: server_3.x/128: Console, Changes, Git Data |
Build 3.6 :: sync-to-downstream_3.x/2543: Console, Changes, Git Data |
Build 3.6 :: get-sources-rhpkg-container-build_3.x/2403: server : 3.x :: Failed in 50990583 : BREW:BUILD/STATUS:UNKNOWN |
Add a parsing rule to detect credentials in factory URLs if it is in a format https://<username>:<pasword>@hostname. Extract the credentials from factory URLs and pass them to the devfile content request.
What does this PR do?
https://<username>:<pasword>@hostname
.Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#21998
How to test this PR?
quay.io/ivinokur/che-server:next
https://<PAT>@github.com/<path to devfile.yaml>
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.