[Utility] trustless relays servicer token validation

adshmh commented 1 year ago

Description

Add validation of application's session tokens to the servicer.

Issue

Part of work on #754

Type of change

Please mark the relevant option(s):

[x] New feature, functionality or library
[ ] Bug fix
[ ] Code health or cleanup
[ ] Major breaking change
[ ] Documentation
[ ] Other

List of changes

Servicer now validates the availability of tokens in the current session before executing relays.

Testing

[x] make develop_test; if any code changes were made
[ ] make test_e2e on k8s LocalNet; if any code changes were made
[ ] e2e-devnet-test passes tests on DevNet; if any code was changed
[ ] Docker Compose LocalNet; if any major functionality was changed or introduced
[ ] k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[] I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
[x] I have tested my changes using the available tooling
[x] I have updated the corresponding CHANGELOG

If Applicable Checklist

[ ] I have updated the corresponding README(s); local and/or global
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] I have added, or updated, mermaid.js diagrams in the corresponding README(s)
[ ] I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

adshmh commented 1 year ago

Will update this PR shortly to add/use Persistence module placeholders for maintaining state.

adshmh commented 1 year ago

@Olshansk @h5law this is now ready for review.

adshmh commented 1 year ago

Also pushed a commit to implement the relay execution.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 47.48% and project coverage change: +0.28 :tada:

Comparison is base (2d4f789) 31.52% compared to head (f3bfe13) 31.80%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #803 +/- ## ========================================== + Coverage 31.52% 31.80% +0.28% ========================================== Files 107 107 Lines 9034 9190 +156 ========================================== + Hits 2848 2923 +75 - Misses 5846 5914 +68 - Partials 340 353 +13 ``` | [Impacted Files](https://app.codecov.io/gh/pokt-network/pocket/pull/803?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network) | Coverage Δ | | |---|---|---| | [utility/servicer/module.go](https://app.codecov.io/gh/pokt-network/pocket/pull/803?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network#diff-dXRpbGl0eS9zZXJ2aWNlci9tb2R1bGUuZ28=) | `53.40% <47.48%> (-8.39%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/pokt-network/pocket/pull/803/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pokt-network)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

gitguardian[bot] commented 1 year ago

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

| GitGuardian id | Secret | Commit | Filename | | | -------------- | ------------------------- | ---------------- | --------------- | -------------------- | | [5841025](https://dashboard.gitguardian.com/incidents/5841025?occurrence=98636881) | Generic High Entropy Secret | 30750fff74deee6ae66a57a6948c754cd90c9da6 | charts/pocket/templates/configmap-genesis.yaml | [View secret](https://github.com/pokt-network/pocket/commit/30750fff74deee6ae66a57a6948c754cd90c9da6#diff-33d68fb410c6b83957f59a2c6fcdc96d62a90d236e6a2a65bf24b205baa46f5dR1799) |

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code. 2. Replace and store your secret safely. [Learn here](https://blog.gitguardian.com/secrets-api-management?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) the best practices. 3. Revoke and [rotate this secret](https://docs.gitguardian.com/secrets-detection/detectors/generics/generic_high_entropy_secret#revoke-the-secret?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). 4. If possible, [rewrite git history](https://blog.gitguardian.com/rewriting-git-history-cheatsheet?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data. To avoid such incidents in the future consider - following these [best practices](https://blog.gitguardian.com/secrets-api-management/?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) for managing and storing secrets including API keys and other credentials - install [secret detection on pre-commit](https://docs.gitguardian.com/ggshield-docs/integrations/git-hooks/pre-commit?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

adshmh commented 1 year ago

@Olshansk the review comments are now addressed, except a few that may need further discussion. I also added a commit to improve the default config behavior, as the introduction of the servicer default config combined with possibly incorrect default config behavior was breaking some of the unit tests.

Olshansk commented 1 year ago

@adshmh regarding the three outstanding comments, please see my replies:

Will take a look at the changes shortly

adshmh commented 1 year ago

@adshmh regarding the three outstanding comments, please see my replies:

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228679186)

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228679364)

* [[Utility] trustless relays servicer token validation #803 (comment)](https://github.com/pokt-network/pocket/pull/803#discussion_r1228705803)

Will take a look at the changes shortly

Thank you for the review comments. Checked and addressed all 3.

adshmh commented 1 year ago

@adshmh Just a few last-minute NITS.

This PR has been open for a while and more changes are being introduced so I'm beginning to get some PR fatigue. After the last few nits are tended to, let's merge it in and follow up in a separate PR if we find issues.

@Olshansk thank you for the reminder and multiple rounds of review. In the most recent commits I have tried to address the remaining review comments through minimum changes, even reverting a config change that had been introduced late in the PR and had not received thorough review.

adshmh commented 1 year ago

@Olshansk sorry for the delay on this. I will address the remaining review comments within 1 day.

adshmh commented 1 year ago

Just a couple of last-minute stylistic NITs but approving since it looks good to me!

Thank you for the detailed review. The 2 remaining review comments are now addressed. I will merge once CI checks pass.

pokt-network / pocket