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

Adding design doc for supporting publishing recipes to insecure registry #30

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

vishwahiremat
Copy link
Contributor

@vishwahiremat vishwahiremat commented Nov 7, 2023

@vishwahiremat vishwahiremat requested review from a team as code owners November 7, 2023 18:24
#### Recipe List/Show
Add a new column to recipe list/show output table.

| NAME | TYPE | TEMPLATE KIND | TEMPLATE VERSION | TEMPLATE | INSECURE-HTTP|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried that we might be scaling too the width of the table too far.

I think we can remove it from rad recipe list for the table format.

And for rad recipe show we may only want to show it on rad recipe show -o json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I'm not sure there's a need to show this in rad recipe list/show. There's a limit to what what we can do with tables. If users need to see every detail, that's what -o json is for.

recipe/2023-11-support-insecure-registries.md Show resolved Hide resolved
```
rad bicep publish --file ./redis-test.bicep --target br:localhost:5000/myregistry/redis-test:v1
```
I would like to see `rad bicep publish` command successfully publish bicep file to locally hosted registry and also I should be able to read it from the registry during recipe deployment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feature! Will be very helpful in local testing!

recipe/2023-11-support-insecure-registries.md Outdated Show resolved Hide resolved

## Alternatives considered

#### Automatically identify the insecure registry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to explore if we can special-case localhost so insecure-http is set to true automatically. The user can still set it to false manually for the edge case where they roll their own SSL certificate locally. And it will allow the publish and register commands to work be default the first time instead of getting an error message.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially because a user won't know there's a problem for Recipe registration until a Recipe deployment is attempted. Having it "just work" without waiting until a deployment to see the error message is a much better user experience.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context the oras client special cases localhost to use insecure http.

Copy link
Contributor

@rynowak rynowak Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to explore if we can special-case localhost so insecure-http is set to true automatically. The user can still set it to false manually for the edge case where they roll their own SSL certificate locally. And it will allow the publish and register commands to work be default the first time instead of getting an error message.

I want to argue about this 😆 because I don't think it's useful.

For locally-hosted registries the pull and push URLs are asymmetric. rad recipe publish uses localhost, and recipe registration DOES NOT use localhost. Users still deal with the same level of complexity because Kubernetes is involved. I don't think the heuristic helps.

Our usecase is different from ORAS' use case. I'd rather be explict, and I don't think this detail is worth explaining to users, it feels like noob-trap.

There's also no such thing as setting a command-line flag to false, so it requires a different design. Security should be opt-out rather than opt-in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we also thought about possible security issues insecure registries may bring?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason, would it be good to restrict insecure registries to just localhost?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why weaking security is explicit for users. It's not a security issue if it's communicated clearly and the user understands the choice they are making.

To paraphrase one of my security-expert friends:

Good security features are usable. If security features are not usable, then users will find bad alternatives.

The user is able to understand their scenario and whether it's appropriate. Our code is general purpose and can't make assumptions.

### Design details
Since we use ORAS client to communicate with the registry, it provides an option "plain-http" to allow insecure connections to registry without SSL check. `plainHttp` property should be set to `true` when client to the remote repository is created to support using insecure registries.

#### Introduce "insecure-http" opt-in flag for users.
Copy link
Contributor

@kachawla kachawla Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we considered alternative parameter names that more explicitly describe the action being taken, such as skipTLSVerification? The term insecure can be somewhat vague and might inadvertently give a negative impression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today docker uses "--insecure" flag and ORAS uses "--plain-http" flag to be set if user wants to use insecure registry.
I and @rynowak discussed on this and decided on using --insecure-http to be explicit to the users that they would be using http link i.e insecure but many of us were leaning towards using --plain-http flag to be consistent with ORAS and since we would be setting plainHttp flag in the backend.
@ryanwaite wanted to know your thoughts on this.
cc: @kachawla, @AaronCrawfis , @willtsai

Copy link
Contributor Author

@vishwahiremat vishwahiremat Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an offline discussion with @ryanwaite and agreed upon using --plain-http flag. Will update it in the doc

Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
@vishwahiremat vishwahiremat merged commit a0de448 into main Dec 5, 2023
2 checks passed
@vishwahiremat vishwahiremat deleted the vishwahiremat/support-insecure-registries branch December 5, 2023 19:03
sk593 pushed a commit to sk593/design-notes that referenced this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants