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

Aryeh/rpc-auth #36

Merged
merged 58 commits into from
Nov 25, 2020
Merged

Aryeh/rpc-auth #36

merged 58 commits into from
Nov 25, 2020

Conversation

harryttd
Copy link
Collaborator

@harryttd harryttd commented Oct 29, 2020

  • Install devspace
  • mkchain generate-constants....
  • devspace use namespace tqtezos
  • cd ./docker/rpc-auth
  • devspace dev --var=CHAIN_NAME="$CHAIN_NAME"
  • (If you want to pass more mkchain args, you can do --var=CHAIN_ARGS="--docker-image=tezos/tezos:v8-release ...")

The above will build (if not already built) the python and zerotier docker images, and apply the yaml for mkchain and rpc auth server. It also enables the minikube nginx addon so that the rpc-auth ingress can work.

Thoughts/Comments on first iteration of this auth mechanism:

  • If this will turn into a separate repo, we need to especially keep in mind that the namespaces are consistent across them so that all the services/pods can talk to each other (easily).
  • Clients can request as many new url's as they want.
  • Need to understand more what potential attack vectors there are and how to prevent. (I believe a replay attack with regards to signing the server-sent-nonce is avoided, by running the getting and deleting of the nonce (that we are verifying for) in a redis transaction.)
  • Probably need to configure redis in more detail. For example, how often it persists data.

Run `k create cm rpc-auth-config -n tqtezos1 --from-file rpc-auth/server/ --dry-run -o yaml | k apply -f -` to update the config map which will then contain the python script.

Sometimes if you make updates locally to the file and run the above command, the pod will restart the server bec it's in debug mode. Most of the times it doesn't and it is unclear why.
- "redis-cli -h $(hostname) ping"
initialDelaySeconds: 5
periodSeconds: 3
# resources:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

k8s noob question: at what point in time do we configure resources for all of our different infrastructure objects? How do we determine what the resources should be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's important to set resources once we are in production and have metrics collection in place. The main benefit is to have sensible autoscaling behaviour. In a minikube context it matters less since there is only one node anyway.

return False


## Need a proper server for production
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to configure a proper server for production

Copy link

Choose a reason for hiding this comment

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

uwsgi!

@harryttd harryttd linked an issue Oct 29, 2020 that may be closed by this pull request
import requests

TEZOS_CHAIN_ID = os.getenv("TEST_CHAIN_ID")
TEZOS_RPC = f"{os.getenv('TEZOS_RPC')}:{os.getenv('TEZOS_RPC_PORT')}"
Copy link

Choose a reason for hiding this comment

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

TEZOS_RPC should be full endpoint, including scheme; as would be accepted by tezos-client's --endpoint paramemter in tezos 8.0; code below has `http hardcoded, which is problematic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TEZOS_RPC is meant for the server to communicate with the tezos rpc k8s service to do its own logic. Not for the client to send their RPC requests. I can rename the variable to be more clear as well as add the scheme to it so that it does not need to be written elsewhere in the code here and here

Copy link

@itkach itkach Oct 30, 2020

Choose a reason for hiding this comment

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

Ah, I see generate_secret_url actually uses request.url_root... I'm not sure how Flask application would know it's public url (must be explicit configuration?), but this assumes that this service would be doing the proxying of actual requests, which it shouldn't

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think at this point the Flask app needs to know its public url. Whatever host the client requests from in order to get a secret url in the first place is the host of the Flask app. They couldn't talk to our app without knowing the host's address. Then we can just use that host as the host of the secret url returned to the client.


@app.route("/tezos-node-rpc/<access_token>/<path:rpc_endpoint>",
methods=["GET", "POST", "PATCH", "DELETE", "PUT"])
def rpc_passthrough(access_token, rpc_endpoint):
Copy link

Choose a reason for hiding this comment

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

this web service should NOT take on proxying requests; correctly proxying all requests is not that simple; this naive implementation is already not correct even for simplest GETs (it doesn't pass along any headers); instead this service should be used together with nginx's auth_request (https://nginx.org/en/docs/http/ngx_http_auth_request_module.html)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now using nginx auth-url annotations to handle the auth subrequest



def verify_chain_id(chain_id):
global TEZOS_CHAIN_ID
Copy link

Choose a reason for hiding this comment

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

no need for global here; would be even better if getting chain id were a function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My intent here is that the server should not have to fetch the chain id on every request that a client makes. I set the CHAIN_ID var as a global variable at the top of the file so that it is kept in memory the whole time the server is alive. Only on the first request to the server will we go and fetch the chain id. I do have a get chain id function that is called a few lines below and is defined under this function.

Without setting global TEZOS_CHAIN_ID i get a local var referenced before assignment error.

Copy link

Choose a reason for hiding this comment

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

Without setting global TEZOS_CHAIN_ID i get a local var referenced before assignment error.

only because you try to assign it; caching function (e.g. using https://docs.python.org/3/library/functools.html#functools.cache) would provide a cleaner implementation (less global mutable state)



def create_nonce():
return str(uuid4().hex)
Copy link

Choose a reason for hiding this comment

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

no need to wrap in str()



def generate_secret_url(public_key):
access_token = str(uuid4())
Copy link

Choose a reason for hiding this comment

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

this gives the same format as create_nonce() but spelled differently, would be nice to be consistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove the create_nonce() func as I don't think it is necessary. create_nonce() right now returns a uuid in hex format as that is required for the tezos-client while here it is plain uuid (cd470ace-264f-418b-95a2-4202bd8956dc). Not sure what you mean by "spelled differently". Would like this function to generate an access token in hex format as well?

Copy link

Choose a reason for hiding this comment

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

the result is exactly the same as far as I can tell - hex string without dashes

❯ python
Python 3.8.6 (default, Oct  8 2020, 14:07:53)
[Clang 11.0.0 (clang-1100.0.33.17)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import uuid
>>> uuid.uuid4()
UUID('b7e0552e-6de4-4d5b-965e-48b9970f52db')
>>> uuid.uuid4().hex
'4f5359ef73f74663897ee9e903be51bc'
>>> str(uuid.uuid4().hex)
'a4d43e241bd640d08098f46fa8dcdab8'
>>> uuid.uuid4().hex
'7259e167589946659a5f0288f32c2d53'
>>>



@app.route("/vending-machine")
def get_tezos_rpc_url():
Copy link

Choose a reason for hiding this comment

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

this should only accept POST method and be called "generate" rather than "get" since it is meant to have side-effects (generating new secret url every time)


def generate_secret_url(public_key):
access_token = str(uuid4())
redis.set(create_redis_access_token_key(access_token), public_key)
Copy link

Choose a reason for hiding this comment

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

this should probably be associated with tz address (pkh) rather than full public key since tz address is the common way of identifying tezos accounts
should probably also add access token to list associated with tz address (along with timestamp) so that we can inspect and, if need be, revoke or expire access codes for a given tz addres

return False


## Need a proper server for production
Copy link

Choose a reason for hiding this comment

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

uwsgi!


def get_chain_id():
response = requests.get(
urljoin(f"http://{TEZOS_RPC}", "chains/main/chain_id"))
Copy link

Choose a reason for hiding this comment

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

This web service in all likelihood may be able to access a Tezos RPC service by it's internal IP, while secret URL generated for the client will use public host/port - so not the same.

@harryttd harryttd removed the request for review from agtilden November 2, 2020 16:49
Node was listening also on ipv6 loopback when using "localhost". Not sure if this matters but for backwards consistency I changed it.
@harryttd harryttd marked this pull request as ready for review November 18, 2020 02:14
@harryttd harryttd requested a review from s-zeng November 18, 2020 02:14
@harryttd
Copy link
Collaborator Author

Taquito and Pytezos both work.

const { TezosToolkit } = require("@taquito/taquito")
const tezos = new TezosToolkit(
  "http://192.168.64.52/tezos-node-rpc/300c4ae403f343d49595076ce7bd97da"
)
tezos.rpc.getChainId().then(console.log)
// NetXvmLTRGRTusY
from pytezos import rpc
rpcNode = rpc.node.RpcNode("http://192.168.64.52/tezos-node-rpc/300c4ae403f343d49595076ce7bd97da")
print(rpcNode.get("chains/main/chain_id"))
# NetXvmLTRGRTusY

@harryttd harryttd requested a review from itkach November 18, 2020 17:26

COPY --chown=appuser:appuser ./server/index.py .

ARG FLASK_ENV=development
Copy link

Choose a reason for hiding this comment

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

prod is probably a better default

@@ -319,6 +324,7 @@ def main():
"bootstrap_timestamp": datetime.utcnow()
.replace(tzinfo=timezone.utc)
.isoformat(),
"rpc_auth": False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is needed here. argparse will set it to false unless the user passes the arg.

--processes 1 \
--wsgi-file index.py \
--worker-reload-mercy 0 \
$(if [ "${FLASK_ENV}" = "development" ] ; then echo "--touch-reload index.py" ; else : ; fi)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that useful considering that devspace will rebuild and restart the container when something changes?

Copy link

@itkach itkach Nov 20, 2020

Choose a reason for hiding this comment

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

@nicolasochem looks like devspace is set up to sync files for rpc-auth rather than rebuild image, so this makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is true

- Client can then make RPC requests:
- `curl http://192.168.64.51/tezos-node-rpc/ffff3eb3d7dd4f6bbff3f2fd096722ae/chains/main/chain_id`
- Bug in tezos client v8, so as of version `tezos/tezos:master_08d3405e_20201113152010`:
- `tezos-client --endpoint http://192.168.64.51/tezos-node-rpc/ffff3eb3d7dd4f6bbff3f2fd096722ae/ rpc get chains/main/chain_id`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also add a paragraph in DEVELOPMENT.md explaining how to spin up rpc-auth on minikube with devspace.

devspace.yaml Outdated
docker:
options:
buildArgs:
FLASK_ENV: ${FLASK_ENV}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this variable also needs to be declared in vars: section below with source: env and a default value otherwise devspace build prompts for the value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicolasochem I just tried with no prompt:

echo $FLASK_ENV

❯ devspace build --skip-push -b
[done] √ Done building image tezos-rpc-auth:qchX3Uf (rpc-auth)
[done] √ Done building image tezos-zerotier:KkAWwY3 (zerotier)
[done] √ Successfully built 2 images

How can I reproduce?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicolasochem yes, removing .devspace prompted me. thanks

apiVersion: v1
kind: Service
metadata:
name: rpc-auth-service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just call it rpc-auth?

@@ -0,0 +1,167 @@
import os
Copy link

Choose a reason for hiding this comment

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

putting rpc-auth under docker directory makes no sense to me; rpc-auth is a distinct component, just like mkchain. If we are not moving it out to a separate repo then rpc-auth and mkchain need to be sibling subdirs in the project, each with its own setup.py. Right now mkchain is "dominant" with its setup.py sitting in project root and rpc-auth is shoved under the rug in a weird place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a container centric view of the environment. mkchain does not belong here because it's not part of the cluster. It's a helper script to generate helm chart values.

See also: https://github.com/midl-dev/tezos-on-gke/tree/master/docker

One of the things I am planning is to migrate generateTezosConfig.py and import_keys.sh into init containers in the docker folder.

Perhaps mkchain could move to the scripts folder that has been added in this PR?

Copy link

@itkach itkach Nov 20, 2020

Choose a reason for hiding this comment

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

mkchain is a software component, so is rpc-auth. They have their own packaging, dependencies, documentation, possible usage and deployment scenarios. I would really like to see component based structure here, perhaps even force it via putting them in separate repos (monorepo can work too, but not like this). Containers flow from that, not the other way around.

@harryttd harryttd linked an issue Nov 20, 2020 that may be closed by this pull request
@nicolasochem
Copy link
Collaborator

nicolasochem commented Nov 20, 2020 via email

@harryttd harryttd merged commit ecd7efa into master Nov 25, 2020
@harryttd harryttd deleted the aryeh/rpc-auth branch November 25, 2020 19:51
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.

Configure redis persistance for RPC authentication Implement Tezos node RPC access via secret link
4 participants