Support pre-signed URLs #157
Conversation
|
Can one of the admins verify this patch? |
machacekondra
force-pushed
the
presigned
branch
2 times, most recently
from
d001eb4 to
318bc18
Compare
| @@ -147,8 +147,9 @@ deploy-on-kind: | |||
| deploy/k8s/migration-planner.yaml.template > deploy/k8s/migration-planner.yaml | |||
| $(KUBECTL) apply -n "${MIGRATION_PLANNER_NAMESPACE}" -f 'deploy/k8s/*-service.yaml' | |||
| $(KUBECTL) apply -n "${MIGRATION_PLANNER_NAMESPACE}" -f 'deploy/k8s/*-secret.yaml' | |||
| @config_server=$$(ip addr show ${IFACE}| grep -oP '(?<=inet\s)\d+\.\d+\.\d+\.\d+'); \ | |||
| $(KUBECTL) create secret generic migration-planner-secret -n "${MIGRATION_PLANNER_NAMESPACE}" --from-literal=config_server=http://$$config_server:7443 --from-literal=config_server_ui=https://$$config_server_ui/migrate/wizard || true | |||
| config_server=$$(ip addr show ${IFACE}| grep -oP '(?<=inet\s)\d+\.\d+\.\d+\.\d+'); \ | |||
There was a problem hiding this comment.
In follow up PR we need to refactor the names and env vars.
internal/image/url.go
Outdated
| ImageExpirationTime = 4 * time.Hour | ||
| ) | ||
|
|
||
| func GenerateShortImageDownloadURLByToken(baseUrl string, source *model.Source) (string, *strfmt.DateTime, error) { |
There was a problem hiding this comment.
I'm not nit-picking here but the func name could be much simpler. What does "ShortImage" mean in this context? :)
There was a problem hiding this comment.
So renaming to: GenerateDownloadURLByToken
internal/image/url.go
Outdated
| return token.SignedString(key) | ||
| } | ||
|
|
||
| func ParseExpirationFromToken(tokenString string) (*strfmt.DateTime, error) { |
There was a problem hiding this comment.
FromToken is useless and tokenString could be token.
i know but I couldn't help myself. :)
There was a problem hiding this comment.
Well the tokenString makes sense in method because then we call:
token, _, err := new(jwt.Parser).ParseUnverified(token, jwt.MapClaims{})
to get jwt.Token and that we name the token, I think it nicely says what is what
but I will remove FromToken for sure
internal/image/url.go
Outdated
| func ValidateToken(token string, keyFunc func(token *jwt.Token) (interface{}, error)) error { | ||
| parsedToken, err := jwt.Parse(token, keyFunc) | ||
| if err != nil { | ||
| return fmt.Errorf("Unauthorized") |
There was a problem hiding this comment.
it would be nice to have an error here to be able to add more context in the caller body. Like for which source id was this
internal/service/image.go
Outdated
| return server.GetImageByToken401JSONResponse{Message: "error creating the HTTP stream"}, nil | ||
| } | ||
|
|
||
| ova := &image.Ova{SshKey: source.SshPublicKey, SourceID: source.ID, Writer: writer} |
There was a problem hiding this comment.
please use the builder here. this is obsolete
machacekondra
force-pushed
the
presigned
branch
from
318bc18 to
a2377d1
Compare
| } | ||
|
|
||
| // TODO: How to add the pull secret??? | ||
| size, err := imageBuilder.Generate(ctx, writer) |
There was a problem hiding this comment.
We should add a new option to the builder WithPullSecret
There was a problem hiding this comment.
Yup, but we don't have it...
internal/service/handler.go
Outdated
| return &ServiceHandler{ | ||
| store: store, | ||
| eventWriter: ew, | ||
| cfg: cfg, |
There was a problem hiding this comment.
So I can take Image URL from config.yaml in case env var is not set. This needs refactoring, I put TODO in code, I will do it in follow up PR.
There was a problem hiding this comment.
but we have a new service now. I don't see where this var is used in this service.
There was a problem hiding this comment.
The hostname of the image service is defined either in MIGRATION_PLANNER_URL or in config.yaml
There was a problem hiding this comment.
Ah, only now I see it's wrong service, you are right, removing....
machacekondra
force-pushed
the
presigned
branch
from
a2377d1 to
9a84abf
Compare
machacekondra
force-pushed
the
presigned
branch
3 times, most recently
from
01b4be3 to
bf7bfe2
Compare
This feauture is built on top of: https://github.com/openshift/assisted-service/blob/master/docs/enhancements/image-service-cloud-authentication.md https://github.com/openshift/assisted-service/blob/master/docs/enhancements/short-url-enhancement.md The flow to get URL: GET http://{service}/api/v1/sources/{source-id}/image-url This will generate the URL which could be used for 4hours by user to download the OVA, then the token will be invalidated. Each source keep private key to sign&verify the URL token. Signed-off-by: Ondra Machacek <omachace@redhat.com>
machacekondra
force-pushed
the
presigned
branch
from
bf7bfe2 to
80d0dc6
Compare
This feauture is built on top of:
https://github.com/openshift/assisted-service/blob/master/docs/enhancements/image-service-cloud-authentication.md https://github.com/openshift/assisted-service/blob/master/docs/enhancements/short-url-enhancement.md
The flow to get URL:
GET http://{service}/api/v1/sources/{source-id}/image-url
This will generate the URL which could be used for 4hours by user to download the OVA, then the token will be invalidated.
Each source keep private key to sign&verify the URL token.