Adds CI to the Public Repo by koolzz · Pull Request #76 · sdnfv/openNetVM

koolzz · 2019-03-09T20:14:18Z

This PR adds necessary changes and enables the CI usage from the public repo

Summary:

Allows CI to run in the public repository. This is done by extracting the repo name from the github webhook.

Also adds a few security updates:

Adds white listed IP check from https://api.github.com/meta
Adds secret check from the webhook

Usage:
Create a new PR or tag @onvm to check speed tester stats

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements	👍
Bug fixes
New functionality
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

Dependencies: None

TODO before merging :

PR is ready for review

Test Plan:

Run CI, it currently runs this version so if everything is fine this will be good to merge.

Review:

Review checklist:

Sanity checks, assigned to @dennisafa @kevindweb
- Run make in /onvm and /examples directories
- Test new functionality or bug fix from the pr (if needed)
Code style, assigned to @dennisafa @kevindweb
- Check everything style related
Code design, assigned to @dennisafa @kevindweb
- Check for memory leaks
- Check that code is reusable
- Code is clean, functions are concise
- Verify that edge cases properly handled
Security @nks5295
Documentation, assigned to @dennisafa @kevindweb
- Check if the new changes are well documented, in both code and READMEs

onvm · 2019-03-09T20:14:25Z

In response to PR creation

CI Message

Your results will arrive shortly

koolzz · 2019-03-09T23:33:47Z

@onvm I disabled you right?

koolzz · 2019-03-14T05:01:11Z

@onvm CI for auth user

onvm · 2019-03-14T05:01:19Z

@onvm CI for auth user

CI Message

Your results will arrive shortly

koolzz · 2019-03-14T05:34:07Z

@nks5295 I want to merge this so at least some people can run CI. And we can figure out a better solution later. I disabled the webhook so it won't trigger but if you have time to review, enable the webhook, tag @onvm, it shouldn't allow you to run CI (I'll add you if you approve the pr 😉).

AntonyDrovalov · 2019-03-15T05:09:59Z

@onvm test CI

onvm · 2019-03-15T05:10:03Z

@onvm test CI

CI Message

User not authorized to run CI, please contact one of the repo maintainers

koolzz · 2019-03-15T05:10:39Z

@AntonyDrovalov хорош все работет

rskennedy · 2019-03-20T20:22:23Z

 def init_ci_pipeline():
+
+    if not verify_request_ip(request):
+        print("Incoming webkooh not from a valid Github address")


spelling "webkooh"

kevindweb · 2019-03-21T04:21:44Z

-child.expect("Password.*")
-child.sendline(password + "\n")
+
+if '-dev' in REPO_NAME:


Is this because for openNetVM-dev, there are already permissions? So someone can only pull if they are in our team? Not an edit, just a question.

yep, if you have access to -dev -> it's assumed you can run CI

kevindweb

Tested this in an onvm repo #65 (which didn't have any onvm comments). It ran with my user which is in the config file I see so, perfect! Looking forward to extending features on this in the future.

nks5295 · 2019-03-21T22:33:19Z

@onvm asdfasdf

nks5295 · 2019-03-21T23:02:49Z

 @app.route(EVENT_URL, methods=['POST'])
 def init_ci_pipeline():
+
+    if not verify_request_ip(request):


When a server has to handle authN/authZ, it should maintain audit logs and error logs. These logs are not to be exposed to github, but they are to be maintained internally.

The ideal way to go about this is to maintain some kind of "request context" that holds request specific info such as the github username, incoming ip, unique fields to identify the caller, and something to identify what gh op triggered the call (pr, issue, etc). this ctx should be passed throughout the ci rig for any call.

For every failure, the values from the req ctx and the failure message should be logged in the error log.

authN/authZ failures, for example, all of these verify_request_* calls and the check to see if the user is in the whitelist, should be logged in an audit log. these logs should contain alerts about who failed to log in and who is not authorized, and why they failed to log in.

nks5295 · 2019-03-21T23:10:37Z

+
+    return False
+
+def verify_request_secret(request):


we talked offline, but i would like dealing with the secret to be like this:

secret encrypted when stored at rest on disk

ci server loads secret and enc key

ci server decrypts secret

ci server calculates and stores hash of secret

ci server frees secret, enc secret, and key (ensure mem is free and cleared)

… public_ci_update

koolzz · 2019-03-28T05:17:01Z

@onvm lets go my friend

koolzz · 2019-03-28T05:17:28Z

@onvm now that I've started you, lets go

koolzz · 2019-03-28T05:19:01Z

@onvm now that I have definitely started you, lets go

onvm · 2019-03-28T05:19:09Z

@onvm now that I have definitely started you, lets go

CI Message

Your results will arrive shortly

onvm · 2019-03-28T05:25:58Z

@onvm now that I have definitely started you, lets go

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35207182

Linter Passed

koolzz · 2019-03-28T05:27:21Z

@nks5295 give this a look it has logging and encryption. Should we also null and delete the request from memory (the one obtained from flask)?

nks5295 · 2019-04-01T17:34:49Z

+        return False
+
+    signature = header_signature.split('=')[1]
+    secret = decrypt_secret()


Are we decrypting the secret each time? That's going to become expensive depending on how many times the CI is triggered.

Is it possible to calculate and store the secret digest during server init, and then to use that in this function when verifying request secrets?

nks5295 · 2019-04-01T17:39:09Z

+
+    if (out):
+        print("Can't run CI, another CI run in progress")
+        log_access_granted(request_ctx, "CI busy, posting busy msg")


Access granted will be printed more often than denied. What happens if there is no more disk space for the server to write logs to? Will the server block? Will it die? Can we make access granted logs be printed only if we have a switch in the config file?

Regardless, still worth thinking about log rotation strategy -- how can we leverage either (a) logrotate or (b) have the server do something to manage log size?

nks5295 · 2019-04-01T17:40:23Z

+logging.getLogger('werkzeug').setLevel(logging.ERROR)
+logging.basicConfig(filename="access_log", filemode='a',
+                    format='%(asctime)s, %(name)s %(levelname)s %(message)s',
+                    datefmt='%d-%b-%y %H:%M:%S', level=logging.INFO)


For access denied, log level should be ALERT. For granted, INFO is fine.

koolzz · 2019-04-03T06:12:11Z

@onvm @nks5295 As discussed in slack, implemented the changes. Didn't do the logrotate/log size managing yet but we have a bit of time on that until our node runs out of space 😉

onvm · 2019-04-03T06:12:18Z

@onvm @nks5295 As discussed in slack, implemented the changes. Didn't do the logrotate/log size managing yet but we have a bit of time on that until our node runs out of space 😉

CI Message

Your results will arrive shortly

onvm · 2019-04-03T06:19:19Z

@onvm @nks5295 As discussed in slack, implemented the changes. Didn't do the logrotate/log size managing yet but we have a bit of time on that until our node runs out of space 😉

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35122661

Linter Passed

nks5295 · 2019-04-03T17:52:39Z

-    private_key = RSA.import_key(open(webhook_config['private-key-file']).read())
+    global secret_file_name
+    global private_key_file_name
+    global secret


not used in this func

Add security checks, extend CI to public repo

245ff02

This comment has been minimized.

Sign in to view

koolzz changed the title ~~Adds CI to the Public Repo~~ Adds CI to the Public Repo [Disabled CI for a bit, will resume later] Mar 9, 2019

koolzz added 2 commits March 14, 2019 00:49

Added authorized users

fc3dc19

Authorized user log

bfa5873

This comment has been minimized.

Sign in to view

koolzz added the enhancement 🚀 label Mar 14, 2019

koolzz changed the title ~~Adds CI to the Public Repo [Disabled CI for a bit, will resume later]~~ Adds CI to the Public Repo Mar 15, 2019

rskennedy reviewed Mar 20, 2019

View reviewed changes

kevindweb reviewed Mar 21, 2019

View reviewed changes

kevindweb approved these changes Mar 21, 2019

View reviewed changes

nks5295 requested changes Mar 21, 2019

View reviewed changes

koolzz added 3 commits March 28, 2019 00:24

Add secret encryption

0028b57

Add logging

d446af6

Merge branch 'develop' of https://github.com/sdnfv/openNetVM-dev into…

4ee22b4

… public_ci_update

koolzz requested a review from nks5295 March 28, 2019 05:26

nks5295 reviewed Apr 1, 2019

View reviewed changes

Better logging, optimized encryption

0c259ed

nks5295 requested changes Apr 3, 2019

View reviewed changes

Fix unused var

8526e76

nks5295 approved these changes Apr 3, 2019

View reviewed changes

nks5295 merged commit 7ce06fd into sdnfv:develop Apr 3, 2019

koolzz added the CI/Testing 🤖 Continuous Integration and Testing related label May 26, 2019

koolzz added this to the ONVM 19.05 Release milestone May 26, 2019

koolzz deleted the public_ci_update branch June 7, 2019 23:17

Conversation

koolzz commented Mar 9, 2019

Summary:

Merging notes:

Test Plan:

Review:

Review checklist:

Uh oh!

onvm commented Mar 9, 2019

CI Message

Uh oh!

This comment has been minimized.

koolzz commented Mar 9, 2019

Uh oh!

koolzz commented Mar 14, 2019

Uh oh!

onvm commented Mar 14, 2019

CI Message

Uh oh!

This comment has been minimized.

koolzz commented Mar 14, 2019

Uh oh!

AntonyDrovalov commented Mar 15, 2019

Uh oh!

onvm commented Mar 15, 2019

CI Message

Uh oh!

koolzz commented Mar 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindweb left a comment

Choose a reason for hiding this comment

Uh oh!

nks5295 commented Mar 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koolzz commented Mar 28, 2019

Uh oh!

koolzz commented Mar 28, 2019

Uh oh!

koolzz commented Mar 28, 2019

Uh oh!

onvm commented Mar 28, 2019

CI Message

Uh oh!

onvm commented Mar 28, 2019

CI Message

Linter Passed

Uh oh!

koolzz commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koolzz commented Apr 3, 2019

Uh oh!

onvm commented Apr 3, 2019

CI Message

Uh oh!

onvm commented Apr 3, 2019

CI Message

Linter Passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

koolzz commented Mar 28, 2019 •

edited

Loading