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

Add recommendations for secure deployment #41

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

hug-dev
Copy link
Member

@hug-dev hug-dev commented Jun 26, 2020

Also modifies the installation guide to make it a secure deployment
guide using systemd.

Before this is merged, the necessary changes would have to be made across all our repositories:

  • changing the socket location to /tmp/parsec/parsec.sock
  • in the test scripts adding the step to create the /tmp/parsec directory
  • in the Rust Client adding the check for permissions: the /tmp/parsec directory must be owned by parsec user, group-owned by parsec-clients and permissions be 750. This will be secure by default but annoying for testing. We could add a dev feature which does not do that check, but disabled by default.

@hug-dev hug-dev requested a review from ionut-arm as a code owner June 26, 2020 12:13
@hug-dev hug-dev self-assigned this Jun 26, 2020
@hug-dev hug-dev added the security Issues related to the security and privacy of the service label Jun 26, 2020
@hug-dev hug-dev marked this pull request as draft June 26, 2020 12:15
@hug-dev
Copy link
Member Author

hug-dev commented Jun 26, 2020

This is just a draft, I need to test the installation script to make sure the commands work.

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Finally some nice, clear steps for people to use in deployments! 🎉

@@ -30,6 +32,28 @@ Copy and adapt the [configuration](configuration.md) you want to use.
cp parsec/config.toml config.toml
Copy link
Member

Choose a reason for hiding this comment

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

Should the config file be in /tmp/parsec/ with permissions 700?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that folder might be volatile 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! There are other permanent locations where the different files could go, that are used by other services, but the advantage of using the parsec user home folder and /tmp is that the installation steps rely less on using sudo.

Comment on lines 8 to 9
- The service needs to be running as a separate, trusted, user. Ideally the service would be running
as the `parsec` user (O-0).
Copy link
Member

Choose a reason for hiding this comment

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

I think if we do go and implement the check described in the last point, we have to be more strict here and say "The service must be running as the parsec user."

We should also add some notice of this somewhere in the Linux installation guide, if client libraries won't connect at all if these steps arent' taken

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change! Will also add a notice on the clients saying that those steps are essential for clients to successfully connect and verify the service.

@hug-dev
Copy link
Member Author

hug-dev commented Jun 26, 2020

Two remarks I have seen while testing the installation:

  • socket activation is a bit complicated for admins to configure properly and I think not really necessary. I think we should remove it to simplify the installation steps.
  • when the service creates the socket file, if it does not exist, it sets the permissions on it as 755 whereas 666 is needed for it to be read/write by everyone who can see it.

@ionut-arm
Copy link
Member

ionut-arm commented Jun 29, 2020

socket activation is a bit complicated for admins to configure properly and I think not really necessary. I think we should remove it to simplify the installation steps.

I'd be okay with that, maybe park the current state somewhere in case someone asks for this functionality? Or should we hide it behind a feature?

when the service creates the socket file, if it does not exist, it sets the permissions on it as 755 whereas 666 is needed for it to be read/write by everyone who can see it.

That's strange, when I check locally the file has 777 🤔 and I don't know if we can control that, the Rust interface doesn't seem to have anything for that. But maybe we can just go and change the permissions on the file through the filesystem API?

@hug-dev
Copy link
Member Author

hug-dev commented Jun 29, 2020

I'd be okay with that, maybe park the current state somewhere in case someone asks for this functionality? Or should we hide it behind a feature?

Actually, there are no changes needed in the code but only in the systemd unit files! The code can still support that if anyone wants to use their custom unit files and use socket activation.

That's strange, when I check locally the file has 777 🤔 and I don't know if we can control that, the Rust interface doesn't seem to have anything for that. But maybe we can just go and change the permissions on the file through the filesystem API?

I guess it depends how the underlying C library is implemented, if the permission set up for a new socket is not standard 🤔 Yes, I think we can just use the filesystem API to set it ourselves.

I implemented those changes in this draft PR.

@ionut-arm
Copy link
Member

Something tells me the client change is needed first as a new version.

@hug-dev
Copy link
Member Author

hug-dev commented Jun 29, 2020

Something tells me the client change is needed first as a new version.

Yes, all is just for review currently

@hug-dev
Copy link
Member Author

hug-dev commented Jun 29, 2020

Made the client change in this one

Copy link
Contributor

@paulhowardarm paulhowardarm left a comment

Choose a reason for hiding this comment

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

It's good to see this advice being included - I've made some comments.

```

In a deployment **without an Identity Provider**, create the `parsec-clients` group and set the
correct permissions on the socket folder. Mutually trusted Parsec Clients will need to be in that
Copy link
Contributor

Choose a reason for hiding this comment

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

What is meant by "mutually trusted" Parsec clients? The word "mutually" seems to suggest clients that trust each other. But if we really mean clients that are trusted by the service, then we can probably scrub the word "mutually" here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the idea is that the clients trust each other (i.e. they won't use each other's identities) - this is needed because if you have multiple clients and no identity provider, you must have some sort of trust in the other clients

Copy link
Member Author

@hug-dev hug-dev Jun 30, 2020

Choose a reason for hiding this comment

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

To add to what Ionut said, this is directly coming from operational mitigation 10 of the service TM:

A set of mutually trusted clients has restricted read-write access to the service IPC endpoint.

Since any client in that group can access the keys of the other ones, we seemed to think that the trust should be in between the clients.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the following paragraph:

+Similarly to the threat model, this guide proposes different alternatives in case an Identity Provider
+is available or not. The role and description of an Identity Provider in Parsec is described in the
+[System Architecture](https://parallaxsecond.github.io/parsec-book/parsec_service/system_architecture.html) page.
+Currently, Parsec does not support integration with any Identity Provider. To securely install Parsec, please
+follow the steps of deployment **without an Identity Provider**.

mkdir /tmp/parsec
```

In a deployment **without an Identity Provider**, create the `parsec-clients` group and set the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to set a bit of context here around identity provider, which may be an unfamiliar term and could confuse the reader. In fact, since we haven't integrated any identity providers yet, I'm wondering whether we should just assume the absence of it for now, and explain that the socket needs to be protected at deployment time because there is currently no runtime mechanism for Parsec to validate its clients. (We can then explain that future deployments will have more flexible means of protection, and talk about identity providers in that context). Without some context, I think there's a danger that these recommendations could seem arbitrary.

Copy link
Member

@ionut-arm ionut-arm Jun 29, 2020

Choose a reason for hiding this comment

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

Alright! We'll expand on the idea of Identity Provider

Copy link
Member Author

Choose a reason for hiding this comment

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

I am personally more in favour of the option where we leave the Identity Provider sections but expand on what it is and the context. To do so, we can point out to the threat model where we make the same distinction of a deployment with and a without an IP:

Parsec could also be deployed in an environment where all clients trust each other and there is no Identity Provider present, and this would lead to some changes to the Threat Model. These changes will be written in italic.

And also point to the System Architecture where we explain what the Identity Provider is.

Having to create a parsec-clients group and adding each and every client that wants to use parsec in it is quite a burden for administrators so it is nice, I think, for them to see "the light at the end of the tunnel" when an IP would exist.

| 6 | Parsec configuration file should be only writable by the trusted administrator and readable by the service. |
| 7 | The trusted administrator needs to check that during the boot process the trusted identity provider has successfully given the root trust bundle to the service. |
| 8 | The hardware descriptors should only be accessible by trusted privileged processes. |
| 9 | The Listener endpoint should be contained in a location that only the Parsec service and the trusted administrator can access (only them can create the endpoint). |
Copy link
Contributor

Choose a reason for hiding this comment

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

"only them" should be "only they"

| 7 | The trusted administrator needs to check that during the boot process the trusted identity provider has successfully given the root trust bundle to the service. |
| 8 | The hardware descriptors should only be accessible by trusted privileged processes. |
| 9 | The Listener endpoint should be contained in a location that only the Parsec service and the trusted administrator can access (only them can create the endpoint). |
| *10* | *A set of mutually trusted clients has restricted read-write access to the service IPC endpoint.* |
Copy link
Contributor

Choose a reason for hiding this comment

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

Again I'm confused by the sense of "mutual" here. Do we mean that the clients trust each other?

@hug-dev hug-dev force-pushed the secure_deployment branch from 5edef12 to 7f8fa09 Compare June 30, 2020 13:31
The recommendations list what an admin must do to install the service
securely.
Also modifies the installation guide to make it a secure deployment
guide using systemd.
Updates the guide to remove socket activation.

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
@hug-dev hug-dev force-pushed the secure_deployment branch from 7f8fa09 to f898833 Compare July 1, 2020 09:56
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

💯 Let's get this merged!!

@hug-dev hug-dev marked this pull request as ready for review July 1, 2020 10:50
@hug-dev hug-dev merged commit 29cd467 into parallaxsecond:master Jul 1, 2020
@hug-dev hug-dev deleted the secure_deployment branch July 1, 2020 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Issues related to the security and privacy of the service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants