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

Authenticate requests between components #486

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

execjosh
Copy link
Contributor

@execjosh execjosh commented Aug 5, 2022

Use a randomly generated token to authenticate requests to the server for all endpoints, including WebSockets, which will help prevent naïve exploitation of the privilege escalation threat of the /run/ endpoint. The token is passed to the node server via STDIN, which should sufficiently prevent eavesdropping.

There is a new setting that toggles this functionality and is enabled by default.

image

Implementation details

This is implemented using httpCookieStore, which is only available on macOS 10.13+. However this project targets 10.11.

The WKWebView injects the token as a cookie for requests from the foreground and background; so, this change should be transparent to all widgets. Additionally, the HttpOnly flag is set on the cookie, which prevents it from being accessed from JavaScript.

@execjosh execjosh force-pushed the authenticate-component-requests branch from d7f8aea to 4ae4169 Compare August 5, 2022 04:45
@execjosh execjosh force-pushed the authenticate-component-requests branch from 4ae4169 to 31d9be9 Compare August 5, 2022 11:53
@execjosh execjosh force-pushed the authenticate-component-requests branch from 31d9be9 to 5a43805 Compare August 17, 2022 17:42
@@ -166,6 +200,11 @@ - (void)startUp

- (void)shutdown:(Boolean)keepAlive
{
if (shuttingDown) {
Copy link
Owner

Choose a reason for hiding this comment

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

this one is merged as a separate PR already :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased 😄

return dataDictionary;
NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:urlPath];
[request setValue:@"Übersicht" forHTTPHeaderField:@"Origin"];
if (preferences.enableSecurity) {
Copy link
Owner

Choose a reason for hiding this comment

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

might seem like a small thing, but what do you think about having this setting only control whether the server checks the token. It can be sent in all other places regardless.
This way the 'code surface' this setting impacts is reduced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! I'll change the code to do that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

// this will trigger a render
[self->screensController syncScreens:self];

[self fetchState:^(NSDictionary* state) {
Copy link
Owner

Choose a reason for hiding this comment

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

is this one related to this change?

Copy link
Contributor Author

@execjosh execjosh Aug 20, 2022

Choose a reason for hiding this comment

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

Yes. From what I can tell, there is no way to send custom headers with HTTP requests through NSData. The results from NSURLSessionDataTask come asynchronously. So, we need to pass a callback.

Perhaps there is a synchronous way to make an HTTP request with custom headers?

Copy link
Owner

Choose a reason for hiding this comment

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

oh I see, I get the change now (misread at first)!

Use a randomly generated token to authenticate requests to the server for all
endpoints, including WebSockets.  The token is passed to the node server via
STDIN, which should sufficiently prevent eavesdropping.
@execjosh execjosh force-pushed the authenticate-component-requests branch from 5a43805 to 9c7cf76 Compare August 21, 2022 06:56
<button fixedFrame="YES" translatesAutoresizingMaskIntoConstraints="NO" id="4Cm-6a-MW9">
<rect key="frame" x="220" y="265" width="182" height="18"/>
<autoresizingMask key="autoresizingMask" flexibleMaxX="YES" flexibleMinY="YES"/>
<buttonCell key="cell" type="check" title="Enable security measures" bezelStyle="regularSquare" imagePosition="left" state="on" inset="2" id="6iu-kH-BSa">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer to have one setting for each security measure?

Also, if it's fine to have one checkbox, is there a better wording?

Copy link
Owner

Choose a reason for hiding this comment

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

I need to think about it a bit as well. Worst case I can play with it before releasing, so let's just leave as is for now.

@felixhageloh
Copy link
Owner

felixhageloh commented Aug 21, 2022

One thing I just came across is NSHTTPCookieStorage. I also vaguely remember looking at it (and maybe using it) when I first implemented this.
If I understand correctly, adding a cookie to the sharedHTTPCookieStorage will add the cookie to all requests sent by the app - not sure about web sockets, tho. If that's correct it would make the code a bit simpler

@felixhageloh
Copy link
Owner

@execjosh did you see my last comment here by any chance?

@execjosh
Copy link
Contributor Author

Oops, I must have missed it. I will try to have a look at NSHTTPCookieStorage when I get a chance.

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.

3 participants