(spyglass/lenses/html) add allow-forms to sandbox

[norrs]

This adds allow-forms to the iframe sandbox.

Blocked form submission to '' because the form's frame is sandboxed
and the 'allow-forms' permission is not set.

This allow's us to click the <a href=link-to-buildbuddy-invocation target=_blank> which links to streaming build results on buildbuddy, as we use bazel with remote builders on buildbuddy.io and the buil-logs uploaded to S3 only contains this hyperlink.

With this change we allow chrome to handle the login process at buildbuddy.io which requires a form-post to handle login over SSO.

https://web.dev/articles/sandboxed-iframes

Original lens PR: kubernetes/test-infra#10208

Later changes that are similar to this one:

allow same-origin: b9a0167
allow popups: kubernetes/test-infra#23069

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: norrs / name: Roy Sindre Norangshol (0b11741)

[k8s-ci-robot]

Welcome @norrs!

It looks like this is your first PR to kubernetes-sigs/prow 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/prow has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @norrs. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: norrs
Once this PR has been reviewed and has the lgtm label, please assign matthyx for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[netlify]

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	0b11741
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-prow/deploys/670582f04f549a0008e85f4d
😎 Deploy Preview	https://deploy-preview-294--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[norrs]

I thought I had personally signed this CLA under this umbrella, anyhow , updated to commit this under Cisco even if I did this outside working hours.

cncf/gitdm#506 has an update on the contributor listing

[petr-muller]

/ok-to-test

I don't know browsers & web apps enough to judge if this has risks, I hope one of the assigned reviewers has a chance to look

[michelle192837]

I'm not super familiar with the risks of this either, but I can take a look into it.

[norrs]

Here is my thought on the security considerations: (I hope I get this correct):

We already allow it to execute javascript from the iframe, which means it can already do more or less a lot. So allow-forms is not really a huge change.

I suppose an attack vector with allowing allow-forms is that the iframe could mimic an input form for phishing attacks, but would you add a source here in your spyglass towards untrusted sources?
We already allow it to execute javascript inside the iframe since day 1.

And iframe is supposed to already be security hardened, just sandbox is taking this to the next level and starts with a privileged mode (read: zero privileges) and you have to grant the privileges the sandbox (the website inside the iframe) requires to run.

But Im unsure if I understand what's stated in here: kubernetes/test-infra#10208 and Rendering implementation ..

Maybe @Katharine, @paulangton or @BenTheElder could chime in with some knowledge and expertise? 🙏

I suppose maybe even better approach would be if there was a way to tune these allowed sandbox arguments as a configuration option, but I haven't researched how easy/achievable this is.

I also learned something new which might make the PR not needed (?):

At least clicking the link vs right-click-open-in-new-tab or middle-mouse-click is different! The latter doesn't make it bring the sandbox to the new tab.

I can share is that this weird chrome iframe bringing sandbox properties to new sites when reusing a tab that was opened from an sandboxed iframe is quite weird from an user's perspective. I would expect if I visit a new URL in the browser in the tab, that it wouldn't bring with sandbox properties from a site I visited >1 page ago. See buildbuddy-io/buildbuddy#7684 if you are curious how I ended up here.

but if we drop this PR, maybe a security review over allow-popups and allow-same-origin as well is wise? (but im still confused by allow-scripts tho..)

It kinda would suck if the "right-click-open-in-new-tab or middle-mouse-click" is different is considered a browser bug, because it would mean I would have to mark the hyperlink manually and paste it in a new tab manually every time I want to browse build logs 😅

I guess it comes down to what is the threat model?
Would it not be bad to allow-scripts even from an iframe source that you do not trust?

Is it maybe intended to only ensure you embed trusted sources in the spy glass?
Maybe we should add a note about that instead in the architecture.md?

There is also the option of
allow-popups-to-escape-sandbox, but is this better or worse than allow-forms?

(Sorry for the wall of text, I'm on uncomfortable grounds and I'm thinking out loud 😅 )

[michelle192837]

Thanks, that's more detailed than what I was about to post 😅

While we're discussing it, do you have an example link to Spyglass for what you're trying to enable? (I looked at the linked discussion in buildbuddy but didn't see a Spyglass example there or here).

Relevant as well: the discussion on the original PR for allow-same-origin: kubernetes/test-infra#22921

[michelle192837]

My first lean is that if this is already opening a new tab/window and it's clear this is an external link (so go at your own risk), allow-popups-to-escape-sandbox is less broad and less subject to misuse than allow-forms. That said given we have allow-scripts already, I think either shouldn't be much more serious to add.

(I do think separately, we should probably review allow-scripts and allow-same-origin in Spyglass, but that's outside the scope of this PR).

[norrs]

While we're discussing it, do you have an example link to Spyglass for what you're trying to enable? (I looked at the linked discussion in buildbuddy but didn't see a Spyglass example there or here).

So we are running prow inside our business unit at Cisco, so the only thing I can provide is a screenshot where I have hidden a bit things, but it should give you the gist of my problem in a visual presentation:

Relevant as well: the discussion on the original PR for allow-same-origin: kubernetes/test-infra#22921

Interesting, it seems to have been discussed before as well.

kubernetes/test-infra#22921 (review)
We control what we put into the iframe so this should be fine. Not a frontend specialist though.

I have to admit that I would agree with this one, and the threat model should be operator of the prow installation that should be responsible for what it trust and define what to add spyglasses to.

kubernetes/test-infra#22921 (comment)

and also this statement about mixing the allow-scripts with allow-same-origin.

Comes back to : kubernetes/test-infra#10208 and Rendering implementation .
where my mind is 🤷 - I do not know because I do not understand how this remote spy glass and the security model is intended to work.

So I kinda come around to: is it better to document that the threat model is up to the operator of the prow instance to only spyglass sources it trusts?

#294 (comment)

But from the twitter example on https://web.dev/articles/sandboxed-iframes :

When reading this over again, I belive this support my statement about the threat model, and a proper enhancement would be to be able for the operator of the prow installation to define this pr spyglass. This way it can sandbox each spyglass properly according to it needs. But hopefully we could argue that this probably is out-of-scope for this PR?

So then the question remains, do we simply add allow-forms ? But I could live without this PR tho, since I did learn about the "middle-mouse-click" and 🙏 this is not a browser bug 😆

Another thing about the allow-popups-to-escape-sandbox , this is my guess what it might protect us from: a bad-iframe-source who has a symlink that links to another internal website, and you have managed to escape sandbox and you could maybe inject some javascript as well somehow with some clever XSS or something? Im not a pen-tester or by any means any frontend expert, but it is a hunch if I let my fantasy run wild and try to think about bad things.

My mind wandered over this thought as well:
if you embed something on your site, via an iframe, you should kinda trust it. I guess this is why advertisers started running ads via iframes with different domains etc to avoid cookiestealing I believe.

edit: formatting pictures into a quote

[norrs]

Oh, when I write spyglass, maybe I actually mean a lense ..

so maybe I do understand it now, the security enhancement around lenses .. was to be solved in the future.. and I believe my suggestion above might sound like what an improved security enhancement could work? 😅

[BenTheElder]

I don't think we should allow this by default, if we're going to weaken the sandbox in any way this should be opt-in and prow.k8s.io will not opt-in.

Why not write the log link out to a file that renders the URL directly, instead of inside the streaming logs?

[BenTheElder]

(I do think separately, we should probably review allow-scripts and allow-same-origin in Spyglass, but that's outside the scope of this PR).

agreed

[norrs]

I don't think we should allow this by default, if we're going to weaken the sandbox in any way this should be opt-in and prow.k8s.io will not opt-in.

I agree, I believe this is achievable with https://github.com/kubernetes-sigs/prow/pull/296/files , and also turns this PR void/not needed.

Why not write the log link out to a file that renders the URL directly, instead of inside the streaming logs?

🤔 Good question, my hunch is that this allows it to show several logs from artifacts uploaded in the object bucket without having to update the configuration from prow.