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

Why doesn't Octo STS output the error detail? #246

Open
suzuki-shunsuke opened this issue May 5, 2024 · 4 comments
Open

Why doesn't Octo STS output the error detail? #246

suzuki-shunsuke opened this issue May 5, 2024 · 4 comments

Comments

@suzuki-shunsuke
Copy link

Hi, thank you for your great project!
I have a question about this app.

Some error messages of this App aren't helpful.
For example, when this App can't parse trust policy, the App outputs Error: unable to parse trust policy found for "***" but we can't understand why.

e.g.

Run octo-sts/action@6177b4481c00308b3839969c3eca88c96a91775f
Error: unable to parse trust policy found for "foo"
image

I checked the source code then I found this App hides the error detail intentionally.

app/pkg/octosts/octosts.go

Lines 311 to 315 in 1fc549c

if err := yaml.UnmarshalStrict([]byte(raw), tp); err != nil {
clog.InfoContextf(ctx, "failed to parse trust policy: %v", err)
// Don't leak the error to the client.
return status.Errorf(codes.NotFound, "unable to parse trust policy found for %q", trustPolicyKey.identity)
}

Don't leak the error to the client.

But I'm not sure why.
Why does this App hide an error detail?
Do we need to hide Trust Policy from clients?

Error detail would be helpful for troubleshooting.

@suzuki-shunsuke suzuki-shunsuke changed the title Why doesn't Octo STS hide the error detail? Why doesn't Octo STS output the error detail? May 5, 2024
@mattmoor
Copy link
Collaborator

mattmoor commented May 5, 2024

The policy may live in a private repo, and since we haven't authorized the caller we don't want to leak any data back to the caller.

@wlynch had some work to add a validating webhook for policies, which I think may have gotten closed when I moved this into its own org, but we should potentially revisit as a way to check things before they are merged into the repository vs. at STS time.

@suzuki-shunsuke
Copy link
Author

Thank you for your quick answer!

The policy may live in a private repo, and since we haven't authorized the caller we don't want to leak any data back to the caller.

I see. It makes sense.

wlynch had some work to add a validating webhook for policies, which I think may have gotten closed when I moved this into its own org

Are you talking about this pull request?

wlynch closed this by deleting the head repository on Mar 13

@mattmoor
Copy link
Collaborator

mattmoor commented May 5, 2024

Yeah that’s the one.

@wlynch
Copy link
Collaborator

wlynch commented May 6, 2024

Revived at #247!

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

No branches or pull requests

3 participants