debugging
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	DavidBrooks
	Dec 15 2019, 3:00 PM

Description

When the /sec-warning page started being delivered to connections with TLS <1.2, calls to the MW API (in my case, using the .net fx 4.5 infrastructure) that expect an XML return started failing in parse because of an unclosed <IMG> tag (which is valid HTML). I would imagine callers expecting json would fail more dramatically. Eventually I was able to peek the incoming HTML, read the comment at the end, and track down how to configure TLS properly.

It might be more helpful to include an HTML comment in the top of the file, directed at API developers, and suggesting they research their HTTP connections and implement the right security level. I'd be more likely to notice that during debugging. But this is a low priority, I'd admit; there probably aren't many in this situation, and in a couple of weeks it will be moot.

See T240497 for my initial misunderstanding on this.

Details

	Subject	Repo	Branch	Lines +/-
	errorpage: Send a comment on browsersec errors	operations/puppet	production	+14 -9

Customize query in gerrit

Related Objects

Mentioned In: T241309: Add more detailed instructions to the "sec-advice" page
Mentioned Here: T240497: API Querying for XML/JSON, you might get the Browser Connection Security warning HTML page (which is invalid XML)

Event Timeline

DavidBrooks created this task.Dec 15 2019, 3:00 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 15 2019, 3:00 PM

Aklapper added a project: Traffic.Dec 15 2019, 4:35 PM

Restricted Application added a project: SRE. · View Herald TranscriptDec 15 2019, 4:35 PM

Krenair subscribed.Dec 16 2019, 12:35 AM

I'd prefer we talked about the drawbacks of serving the /sec-warning HTML with different HTTP status codes (and blocking misbehaving clients), than doing anything special in the response body for particular API formats. It feels like maybe HTTP 400/426?

• ema moved this task from Backlog to TLS on the Traffic board.Dec 16 2019, 8:06 AM

Indeed. client libraries shouldn't be parsing the response bodies when they are HTTP 4xx. If we are sending this page as HTTP 2xx, that's a mistake on our part.

Looks like changing the status code isn't an option per the below from the duplicate task:

In T240497#5735863, @BBlack wrote:

The way it works is that if the connection isn't using TLSv1.2, the user is served a 302 redirect to /sec-warning on the same domain, which in turn returns a cacheable 200 OK with the HTML warning content and the CT header as text/html; charset=utf-8. There are a lot of gory details in the compromises being made by that solution (vs. eg. we could have returned some kind of 4xx error immediately rather than 302->200), but we've learned this is the best pattern to avoid misbehavior of certain bots and scrapers out there in the world which spam-retry [45]xx return codes.

That the contents of the 200 response don't parse as intended is actually a good thing. If they did parse correctly like the response you were expecting, you might still not notice that anything was wrong, and never upgrade the TLS version you're using. We served these sec-warning interruptions earlier as well at lower-percentages (e.g. some days before this, you should've seen ~10% of your requests failing this way) as an early-warning system, but there is no universal early-warning system for such changes that actually works in the real world (nobody reads or reacts to announcements, and generally they don't pay attention when we break a small percentage of their requests either, but we do what we can). The really awful case is POST requests, and there are some bots out there which do only POST and never GET, and don't really bother to parse the output response either...

The best we could do here is make sure the response has the correct MIME type, which, from checking it just now, is already set correctly: content-type: text/html; charset=utf-8.

As mentioned at T240497, this is working by design and as intended. I'm closing this as duplicate now as I'm unable to find a difference in subject or context between the reports. Direct further comments at T240497.

Krinkle closed this task as a duplicate of T240497: API Querying for XML/JSON, you might get the Browser Connection Security warning HTML page (which is invalid XML).Dec 17 2019, 11:41 PM

I want to dispute the closing of this because I was specifically asked by the other folks in the linked task to open this separately. Can you get together and decide how to handle it? As I said, it's probably moot but that's not for me to decide.

ETA: If you consider the actual suggestion in this report and close it as "won't fix", I'd be more comfortable.

In T240497#5739124, @jcrespo wrote:

[…] maybe file a new ticket with suggestions for improvements on /sec-warning page, if you wanted to do so, so they are easier to understand for developers?

I assume you're referring to this? I didn't see this as an improvement for /sec-warning because you referred to an XML comment being added to the response, which I took to mean the api.php?format=xml response. However that's part of MediaWiki.

Are you suggesting that an HTML comment atop be added the existing HTML response that our CDN layer serves at /sec-warning?

This is response currently starts as follows:

<!DOCTYPE html>\n<html lang="en">\n<meta charset="utf-8">\n<title>Browser Connection Security Issues</title>\n<style> …

Yes, an HTML comment, embedded near the top of the page text, not in the response headers. I called it an XML comment because it's in the XML format. Sorry if that was a misdirection. It probably belongs properly in a <HEAD> element (actually, so does the TITLE).

But, since you mention it - does HTTP allow a free-form comment in the response headers? I didn't think it did.

My suggestion, based on your comment, was for you to provide a suggested alternative text for the HTML page that was better understandable to you (not adding a different content type, like xml or json) based on your comment that the html error message, as I understood, was difficult to understand/parse, as I understood that neither the status code, nor the mime-format could be changed based on T240497.

So I suggested to open a ticket with a better error message. Adding an html comment is probably feasible, but up to the owner (traffic) to decide, and probably not of high priority.

I suggest you to update the title, as "please add a helpful XML comment" will be likely misunderstood (s/XML/HTML) and with a bit more background of why you are asking it (e.g. "so for different mime types, source html is easier to understand"), which I would agree is a reasonable suggestion.

Suggest: near the top of the /sec-warning file, add an HTML comment:

I don't think it's possible to be more direct, given the number of platforms that could use the API.

And, as I said, I agree it's low priority because it only matters for another 2 weeks. Perhaps as a placeholder for the next time the file gets deployed, in a few years?

DavidBrooks renamed this task from /sec-warning page: please add a helpful XML comment explaining why it's being delivered. to /sec-warning page: please add an HTML comment that is more easily visible to API and transport-level inspection/debugging.Dec 18 2019, 2:27 PM

DavidBrooks updated the task description. (Show Details)

Or a patch to template this in. The problem is it's implemented from a standard template for the top 30-40 lines, which isn't specific to this case, in an attempt to standardize our error output templates.

The common bit is at: https://gerrit.wikimedia.org/r/plugins/gitiles/operations/puppet/+/production/modules/mediawiki/templates/errorpage.html.erb
While the sec-warning body specific to this output is at: https://gerrit.wikimedia.org/r/plugins/gitiles/operations/puppet/+/production/modules/varnish/templates/browsersec.body.html.erb
And various templating mechanisms glue them together, and you can follow some of that trail from e.g.:
https://gerrit.wikimedia.org/r/plugins/gitiles/operations/puppet/+/production/modules/varnish/manifests/common/browsersec.pp

I think this could be refactored to template a case-specific comment into the shared header, but it's a little more work than just injecting the comment into a static file in our repo.

Ah, that makes it more complex. Again, I guess there's no evidence that anyone else is encountering the API failures due to an out-of-date security infrastructure; AWB for example still uses fx 4.5 but the code must have been fixed well back, so all seems well. I know I've been heard, so you can put this on infinite backlog.

Thanks to all for the patience in addressing this. My thought processes must have slowed down somewhat since retirement. I'll still be grumpy about the missing <HEAD> element though.

jcrespo mentioned this in T241309: Add more detailed instructions to the "sec-advice" page.Dec 26 2019, 11:00 AM

The swap of Traffic for Traffic-Icebox in this ticket's set of tags was based on a bulk action for all such tickets that haven't been updated in 6 months or more. This does not imply any human judgement about the validity or importance of the task, and is simply the first step in a larger task cleanup effort. Further manual triage and/or requests for updates will happen this month for all such tickets. For more detail, have a look at the extended explanation on the main page of Traffic-Icebox . Thank you!

BBlack moved this task from Backlog to Close or Untag? on the Traffic-Icebox board.Apr 13 2022, 8:31 PM

Change 877227 had a related patch set uploaded (by BCornwall; author: BCornwall):

[operations/puppet@production] errorpage: Send a comment on browsersec errors

https://gerrit.wikimedia.org/r/877227

gerritbot added a project: Patch-For-Review.Jan 9 2023, 5:10 PM

BCornwall changed the task status from Open to In Progress.Jan 9 2023, 6:06 PM

BCornwall claimed this task.

Change 877227 merged by BCornwall: