| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-01 | |||
| 18:36:27 | melwitt | I'll take a look in novnc | |
| 18:37:03 | mnaser | Okay | |
| 18:37:11 | melwitt | I thought the cookie thing was a backup, if the token is not in the GET request params, but forwarding via cookie does sound really familiar | |
| 18:37:16 | mnaser | melwitt: we use vnc_lite.html in OSA. https://github.com/novnc/noVNC/commit/51f9f0098d306bbc67cc8e02ae547921b6f6585c | |
| 18:38:14 | melwitt | I had dug into this stuff back when I worked on the db backend for console auths, back when we were thinking we could pass the instance uuid in the GET request too (turns out bc of novnc limitations, more than one request param would not work) | |
| 18:38:50 | melwitt | (it only forwards the first request param) | |
| 18:39:05 | mnaser | So it always probably used a cookie but that seems to have been pulled out from vnc_lite.html which is for some historic reason what’s being used in OSA | |
| 18:40:14 | melwitt | huh, interesting | |
| 18:40:30 | mnaser | The worrying thing is I can’t find any token references in novnc | |
| 18:40:32 | melwitt | / If a token variable is passed in, set the parameter in a cookie. | |
| 18:40:33 | melwitt | / This is used by nova-novncproxy. | |
| 18:40:49 | mnaser | Where do you see this melwitt ? | |
| 18:40:51 | melwitt | seeing that all in red being removed. "used by nova-novncproxy" :P | |
| 18:41:03 | melwitt | mnaser: in the commit you linked https://github.com/novnc/noVNC/commit/51f9f0098d306bbc67cc8e02ae547921b6f6585c#diff-1d6838e3812778e95699b90d530543a1L173 | |
| 18:41:20 | mnaser | ugh | |
| 18:41:42 | mnaser | What does devstack use | |
| 18:42:04 | mnaser | I wonder how it hasn’t been affected | |
| 18:42:15 | melwitt | must not be using lite? | |
| 18:43:07 | melwitt | bc this was removed from the vnc_lite.html file, unless I'm missing something | |
| 18:43:45 | mnaser | Of course | |
| 18:43:58 | mnaser | NOVNC_FROM_PACKAGE defined in jobs | |
| 18:44:43 | melwitt | http://logs.openstack.org/65/627765/12/check/tempest-full-py3/b00b2ab/controller/logs/devstacklog.txt.gz#_2019-04-01_08_01_25_051 | |
| 18:45:17 | mnaser | Yikes well i guess we can ask novnc to revert | |
| 18:49:53 | melwitt | mnaser: I don't understand, how do you use lite vs not? | |
| 18:50:23 | mnaser | melwitt: vnc_lite seems to be what vnc_auto was a while ago | |
| 18:50:36 | mnaser | Searching the code doesn’t give any results for token | |
| 18:50:40 | melwitt | mnaser: and which one you use depends on the url you use? | |
| 18:51:05 | melwitt | yeah, ok. looks like we have to ask for a revert of at least that part | |
| 18:51:22 | mnaser | melwitt: I assume we’re using the lite which handled the token stuff and did the cookie handoff | |
| 18:51:45 | melwitt | mnaser: yeah, I'm just saying, how do you choose whether to use lite or not | |
| 18:51:56 | mnaser | dunno :P | |
| 18:52:01 | melwitt | haha ok | |
| 18:52:13 | mnaser | It seems like there’s something called token plugin | |
| 18:52:24 | melwitt | at first I was thinking, can we use non-lite but then you pointed out only lite was handling token | |
| 18:52:30 | melwitt | so my thought was moo | |
| 18:52:49 | mnaser | Don’t quote me on this stuff I’m going over it on my phone heh | |
| 18:53:30 | melwitt | :) | |
| 18:54:43 | melwitt | I do wonder why we have to rely on the cookie forwarding. I wonder why it doesn't just show up as a GET request param by the time we get it | |
| 18:54:59 | melwitt | from the way our code is written, it looks like the expectation was that the cookie was a fall back | |
| 18:55:14 | melwitt | but in practice, we were always falling back | |
| 18:56:41 | melwitt | going to see if I can find out | |
| 19:15:57 | openstackgerrit | Eric Fried proposed openstack/nova master: Correct lower-constraints.txt and the related tox job https://review.openstack.org/622972 | |
| 19:16:34 | openstackgerrit | Eric Fried proposed openstack/nova master: Correct lower-constraints.txt and the related tox job https://review.openstack.org/622972 | |
| 19:17:38 | openstackgerrit | Eric Fried proposed openstack/nova master: Mock time.sleep() in unit tests https://review.openstack.org/648762 | |
| 19:25:36 | mnaser | melwitt: so I think the problem here is the /websockify HTTP request comes afterwards | |
| 19:26:01 | mnaser | so its GET /vnc_lite.html?token=foobar .. and then CONNECT /websockify | |
| 19:26:12 | mnaser | so you can't really reliably tie that GET request and the CONNECT request | |
| 19:26:28 | mnaser | UNLESS we sent a Set-Cookie: token when you visit vnc_lite.html | |
| 19:26:36 | mnaser | that would workaround it | |
| 19:27:18 | melwitt | mnaser: I see. well, from some googling it looks like something like this might work? GET /vnc_lite.html?path=/websockify?token=foobar | |
| 19:28:19 | openstackgerrit | Eric Fried proposed openstack/nova master: Add placement as required project to functional py36 and 37 https://review.openstack.org/649068 | |
| 19:28:29 | melwitt | second ? should probably be a & | |
| 19:28:45 | mnaser | I think it would be cleaner to set-cookie | |
| 19:28:59 | melwitt | or maybe not | |
| 19:30:02 | melwitt | mnaser: ok. I can see if I can figure out how to do that, unless you want to | |
| 19:32:32 | mnaser | melwitt: im looking around right now, just need to figure out where the HTTP requests are handled in nova-novncconsole | |
| 19:32:35 | mnaser | ill welcome all help :P | |
| 19:32:44 | mnaser | *novncproxy | |
| 19:34:15 | mnaser | think it relies on upstream this https://github.com/novnc/websockify/blob/master/websockify/websocketproxy.py#L267 | |
| 19:34:26 | mnaser | self.token_plugin = kwargs.pop('token_plugin', None) | |
| 19:34:28 | mnaser | oh I wonder what that is | |
| 19:35:39 | melwitt | mnaser: I looked at that and it doesn't seem like something that would help us. but I could be wrong https://github.com/novnc/websockify/wiki/Token-based-target-selection | |
| 19:37:42 | mnaser | maybe we somehow override the html portion | |
| 19:37:42 | mnaser | https://github.com/novnc/websockify/blob/master/websockify/websocketserver.py#L35-L52 | |
| 19:37:48 | melwitt | mnaser: I think handling of HTTP (GET) requests might be this https://github.com/novnc/websockify/blob/6e09ec25486f431e2acc463f994c4eb4bebc6ddd/websockify/websockifyserver.py#L268 | |
| 19:37:57 | mnaser | oh that's even better | |
| 19:38:26 | melwitt | one of those, I don't really understand it yet | |
| 19:39:08 | efried | This one came across my desk again just now, can I please get a couple of pairs of core eyeballs on it? https://review.openstack.org/#/c/635533/ | |
| 19:41:10 | mnaser | I'm thinking https://github.com/novnc/websockify/wiki/Token-based-target-selection#custom-plugins | |
| 19:43:08 | melwitt | mnaser: hm. so instead of validating the token in new_websocket_client we'd do it in a new lookup method? | |
| 19:43:24 | melwitt | 'lookup' | |
| 19:43:29 | mnaser | melwitt: yeah, if we write a custom plugin, we can validate the token in it and then give it back to novnc | |
| 19:43:33 | efried | mriedem: This looks like a deadlock starting n-api? http://logs.openstack.org/36/649036/1/check/tempest-full-py3/a8c7223/controller/logs/screen-n-api.txt.gz | |
| 19:43:53 | melwitt | mnaser: it says lookup has to return a host:port tuple? | |
| 19:44:31 | mnaser | or none, none (if we don't validate I guess) | |
| 19:44:43 | efried | (maybe not, I guess the locks are being released quickly) | |
| 19:44:55 | mnaser | and the host:port tuple business can be grabbed from the db | |
| 19:46:27 | mnaser | the example module there seems to be reasonable | |
| 19:47:20 | mnaser | ugh but even then | |
| 19:47:26 | mnaser | I don't think anything will pass ?token=foo | |
| 19:48:08 | melwitt | mnaser: ok, I was trying to say when you said "give the token back to novnc" I don't think you can do that in lookup() | |
| 19:48:51 | mnaser | oooo | |
| 19:48:58 | melwitt | mnaser: earlier I was saying maybe we could do the entire validation in lookup() and then no longer need the token after that? | |
| 19:49:02 | mnaser | melwitt: https://github.com/novnc/websockify/blob/6e09ec25486f431e2acc463f994c4eb4bebc6ddd/websockify/websocketproxy.py#L164-L175 | |
| 19:49:16 | melwitt | yeah | |
| 19:49:47 | mnaser | so we get lookup called with the token, grab the stuff from the db, and return it back, and we can probably drop all that extra code | |
| 19:49:59 | melwitt | what do you mean by return it back? | |
| 19:50:23 | mnaser | so in the token plugin, we check the db, see if the var is still valid, and return (host, port) | |
| 19:50:36 | mnaser | and technically... I think none will just do the rest of the issues | |
| 19:50:43 | melwitt | ok. I thought you meant return the token | |
| 19:50:45 | melwitt | ok. that's what I was trying to say earlier | |
| 19:50:53 | mnaser | ah no I meant return the host/port, sorry distracted | |
| 19:51:00 | mnaser | people keep walking into my office >:( | |
| 19:51:09 | melwitt | how dare they! | |
| 19:52:39 | mnaser | let me try to hack at this | |
| 19:54:12 | melwitt | sounds good. I will review it when it's ready :) | |
| 19:54:27 | mnaser | honestly I think this could completely eliminate the websocketproxy.py | |
| 19:54:53 | mnaser | a lot of that stuff exists in webproxify but I dunno how much of this can be back portable at this point.. | |
| 19:55:28 | melwitt | maybe. there is some extra stuff in there though | |
| 20:19:18 | mnaser | melwitt: well, I got a nice refactor out of it, but still I think it expects I to be there | |