Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

doc: REST API authentication #1021

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

doc: REST API authentication #1021

wants to merge 1 commit into from

Conversation

aravindavk
Copy link
Member

Closes: #1019
Signed-off-by: Aravinda VK [email protected]

if __name__ == "__main__":
user = "glustercli"
secret_file = sys.argv[1]
secret = open(secret_file).read()
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 its better to use:

with open(secret_file) as f:

instead of open().read() since file getting closed immediately is not assured. After python 3.2 it will thow a warning.ResourceWarning

Copy link
Member Author

Choose a reason for hiding this comment

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

this is only development tool, errors are not handled

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 will update the tool

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@rishubhjain
Copy link
Contributor

@aravindavk The document looks good, maybe we can add another heading stating the error received by the user if the auth fails or if proper authentication steps are not followed. It can be a part of another document (capturing all errors and their solutions) as well.

Closes: #1019
Signed-off-by: Aravinda VK <[email protected]>
claims['iat'] = datetime.utcnow()
claims['exp'] = datetime.utcnow() + timedelta(seconds=10)

token = jwt.encode(claims, secret, algorithm='HS256')
Copy link
Member

Choose a reason for hiding this comment

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

This looks fine, To provide more security, we need to add URL tampering protection also
@aravindavk any thoughts on this

in the server side, we need to enable (HTTP method) validation to avoid miss use of the token.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense. Please open new issue for the same.

## Default user

When `glusterd2` starts for the first time, it creates the
`$GLUSTERD2_STATEDIR/auth` file which will contain the secret. If
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we document anywhere what GLUSTERD2_STATEDIR path mean? Might be good to point out to a link.

Copy link
Member

@kshlm kshlm Jul 18, 2018

Choose a reason for hiding this comment

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

Do we document anywhere what GLUSTERD2_STATEDIR path mean? Might be good to point out to a link

I don't think we have a document about the paths yet. We should do it.

Apart from that, I have a concern about using the GD2 secret by external tools.

The secret is meant to be accessible only by GD2. GD2 should be using this secret to create and handout tokens. Having the clients access the secret is a big security risk. Later in the document, we ask that the secret be copied out externally and have external client generate their own token. This is a serious lapse of security. Because the secret can be passed along to unintended users and those users would have full access to the GD2 cluster.

Ideally, as I mentioned earlier GD2 should be the one generating and handing out tokens. Even for glustercli it would be preferrable that you use token. GD2 should generate a token for CLI and save it to a known place. And we should add a way to generate tokens to GD2 (ReST or CLI to be decided).

Sorry about bringing this up now. I should have been paying attention when the actual PR was being reviewed. I'll open up a new issue about glustercli not using the secret file directly. We can merge this PR without waiting for that, but only if it is modified to address my comments later on.


When `glusterd2` starts for the first time, it creates the
`$GLUSTERD2_STATEDIR/auth` file which will contain the secret. If
"gluster" user group is available in the system then this `auth` file
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we mention how to create "gluster" user group?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do this. This is simple unix knowledge. In any case, if installed from packages, the package should take care of creating the users/groups. I don't think any gluster packages do this yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worthwhile to point out this is a unix group interacting with file permissions and not some special gluster thing in that case. The word group and phrase "in that machine" alone may not hint at that strongly enough.

GLUSTERD2_AUTH_SECRET (environment variable)
--secret-file (default path)

## Curl example
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be under ReST API section?

Copy link
Member

Choose a reason for hiding this comment

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

A link should be added from the ReST API document to this doc.

## Default user

When `glusterd2` starts for the first time, it creates the
`$GLUSTERD2_STATEDIR/auth` file which will contain the secret. If
Copy link
Member

@kshlm kshlm Jul 18, 2018

Choose a reason for hiding this comment

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

Do we document anywhere what GLUSTERD2_STATEDIR path mean? Might be good to point out to a link

I don't think we have a document about the paths yet. We should do it.

Apart from that, I have a concern about using the GD2 secret by external tools.

The secret is meant to be accessible only by GD2. GD2 should be using this secret to create and handout tokens. Having the clients access the secret is a big security risk. Later in the document, we ask that the secret be copied out externally and have external client generate their own token. This is a serious lapse of security. Because the secret can be passed along to unintended users and those users would have full access to the GD2 cluster.

Ideally, as I mentioned earlier GD2 should be the one generating and handing out tokens. Even for glustercli it would be preferrable that you use token. GD2 should generate a token for CLI and save it to a known place. And we should add a way to generate tokens to GD2 (ReST or CLI to be decided).

Sorry about bringing this up now. I should have been paying attention when the actual PR was being reviewed. I'll open up a new issue about glustercli not using the secret file directly. We can merge this PR without waiting for that, but only if it is modified to address my comments later on.


When `glusterd2` starts for the first time, it creates the
`$GLUSTERD2_STATEDIR/auth` file which will contain the secret. If
"gluster" user group is available in the system then this `auth` file
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do this. This is simple unix knowledge. In any case, if installed from packages, the package should take care of creating the users/groups. I don't think any gluster packages do this yet.

GLUSTERD2_AUTH_SECRET (environment variable)
--secret-file (default path)

## Curl example
Copy link
Member

Choose a reason for hiding this comment

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

A link should be added from the ReST API document to this doc.



## Using REST APIs from outside the Cluster nodes

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer you remove this section completely for now. Or at least modify such that it doesn't involve copying out the secret file. The glustercli example would need to be completely removed. For the curl example, you should change it so that the python script is used to generate a token on one of peers, and the token is copied over.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense. I updated the same in other comment #1021 (comment)

For the curl example, you should change it so that the python script is used to generate a token on one of peers, and the token is copied over.

We are following shared secret approach. No change required for curl example unless we decide to do shared token approach again.

@aravindavk
Copy link
Member Author

I think confusion here is user documentation vs developer documentation. I should convert this as developer documentation since users need not make any change to glustercli arguments if deployment is unchanged.(Like custom localstatedir)

I will remove the last section which talks about the temporary arrangement to use this from outside the cluster. Once user management is implemented then they can use those credentials for making REST calls.

Till the user management is implemented, we can't use these REST APIs from outside the cluster.

@aravindavk
Copy link
Member Author

Added comment about Shared token vs Shared secret approach here #1030

@aravindavk
Copy link
Member Author

Apart from that, I have a concern about using the GD2 secret by external tools.

This is temporary till the user management is in place. Once we implement user management, only glustercli will use the secret file. Future REST API documentation may looks like,

  • Create user/app using glustercli useradd <username> which will prompt for secret and creates the user. For example: glustercli useradd webui
  • Once the user is created, REST APIs can be consumed from any node by authenticating using the credentials specified in previous command. For example, glustercli --user webui --secret-file=~/.gluster/webui.secret peer status. Secret can be specified using --secret, --secret-file, env var etc.


## glustercli

With default installation, `glustercli` will know the location of
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, "The glustercli command knows the default location of the auth file." would read more clearly.


With default installation, `glustercli` will know the location of
`auth` file. `glustercli` will generate JWT token using the secret
available in `auth` file and attach it with every REST API calls.
Copy link
Contributor

Choose a reason for hiding this comment

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

"every REST API call." or "all REST API calls."


Default installation will not require any change to use `glustercli`.

If `auth` file is in different path(When `glusterd2` is running with
Copy link
Contributor

Choose a reason for hiding this comment

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

If the auth file is in a different path (for example, when glusterd2 is running with a custom workdir)...

glustercli --secret-file=/root/setup1/glusterd2/auth peer status

**Note**: bash alias can be added like `alias glustercli='glustercli
--secret-file=/root/setup1/glusterd2/auth`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it might be a little out of place to teach people about bash aliases in the doc?

--secret
--secret-file
GLUSTERD2_AUTH_SECRET (environment variable)
--secret-file (default path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting --secret-file here again seems confusing from a user perspective IMO. Just don't mention it and say "default path". It may play into the implementation but I don't think that's relevant to the person reading the doc.


**Note**: Change the path of script and auth file to match your setup.

Thats all, use `gcurl` wherever `curl` is necessory. For example, to start a Volume
Copy link
Contributor

Choose a reason for hiding this comment

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

necessary not necessory

Install `jwt` library using `pip install jwt` or using `dnf install
python-jwt`. Generate JWT token using,

import jwt
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to enclose this block with ````python` esp. if its expected this will be viewed using github/github-style-markdown-renderer.

@Madhu-1
Copy link
Member

Madhu-1 commented Oct 26, 2018

@aravindavk anything pending on this PR?

@Madhu-1
Copy link
Member

Madhu-1 commented Dec 19, 2018

@aravindavk ping, can you address review comments and resolve the merge conflict so that we can get this PR in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants