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

lib/uplinkc: Add and export restrict_api_key #3417

Closed
wants to merge 24 commits into from
Closed

lib/uplinkc: Add and export restrict_api_key #3417

wants to merge 24 commits into from

Conversation

TopperDEL
Copy link
Contributor

@TopperDEL TopperDEL commented Oct 30, 2019

With this PR the function "restrict_api_key" is made available to uplinkc.

What: Include "restrict_api_key" within uplinkc

Why: uplinkc lacks that feature currently

Please describe the performance impact: none

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@TopperDEL TopperDEL requested a review from a team October 30, 2019 11:55
@cla-bot cla-bot bot added the cla-signed label Oct 30, 2019
@ghost ghost requested review from isaachess and jenlij and removed request for a team October 30, 2019 11:55
mniewrzal
mniewrzal previously approved these changes Oct 30, 2019
@stefanbenten
Copy link
Contributor

@TopperDEL Is it on purpose to be made against another branch instead of master?

@TopperDEL
Copy link
Contributor Author

I'm not so used with GitHub - but the base for my PR is the branch of bryan that is not yet merged - so i wanted to merge into his branch...

@egonelbre egonelbre changed the base branch from bryan/uplinkc to master October 30, 2019 12:40
Copy link
Member

@egonelbre egonelbre left a comment

Choose a reason for hiding this comment

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

Some of the review comments are more directed towards @bryanchriswhite.

lib/uplinkc/caveat.go Outdated Show resolved Hide resolved
lib/uplinkc/caveat.go Outdated Show resolved Hide resolved
lib/uplinkc/uplink_definitions.h Outdated Show resolved Hide resolved
lib/uplinkc/uplink_definitions.h Outdated Show resolved Hide resolved
lib/uplinkc/uplink_definitions.h Outdated Show resolved Hide resolved
lib/uplinkc/apikey.go Outdated Show resolved Hide resolved
lib/uplinkc/caveat.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@TopperDEL TopperDEL left a comment

Choose a reason for hiding this comment

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

I need some help with in-depth adjustments to the code.

@mniewrzal
Copy link
Contributor

@TopperDEL I will try to help but I'm not sure I will have time today.

@TopperDEL
Copy link
Contributor Author

Thanks! Does not have to be today.

@TopperDEL
Copy link
Contributor Author

@mniewrzal Can we schedule a call for Friday at around 9 o'clock (GMT) which should be 8 o'clock (UTC)?

@mniewrzal
Copy link
Contributor

@mniewrzal Can we schedule a call for Friday at around 9 o'clock (GMT) which should be 8 o'clock (UTC)?

yeah, sounds good

@TopperDEL
Copy link
Contributor Author

@mniewrzal I'm ready. How can we meet up?

@mniewrzal
Copy link
Contributor

@mniewrzal I'm ready. How can we meet up?

hey, lets try this https://meet.google.com/tsh-giaa-kvk?authuser=1

@TopperDEL TopperDEL changed the title Export restrict_api_key lib/uplinkc: Add and export restrict_api_key Nov 8, 2019
@TopperDEL
Copy link
Contributor Author

This should be finished now. Can someone else have a look at it?
@mniewrzal @egonelbre @bryanchriswhite

@jenlij jenlij removed their request for review November 12, 2019 19:10
@ifraixedes
Copy link
Member

@TopperDEL I wanted to give you a heads up. We have a few people unavailable for several days due to different personal reasons, so we are not forgetting about this but the review will take more time than usual.

Our apologies.

@ifraixedes ifraixedes added the Request Code Review Code review requested label Nov 20, 2019
@TopperDEL
Copy link
Contributor Author

That's ok, Thanks for the heads-up! @ifraixedes

@TopperDEL
Copy link
Contributor Author

Any news on this one?

@mniewrzal
Copy link
Contributor

Any news on this one?

@TopperDEL I'm super sorry for the delay. W discuss how we should proceed with exposing sharing capabilities with uplinkc and we need to do this around Scope type. I pushed initial PR for that (#3689 ). Next step will be to add something like:

func restrict_scope(scopeHandle C.ScopeRef, caveat C.Caveat, restrictions **C.EncryptionRestriction, cerr **C.char) C.ScopeRef 

similar to your function but needs to cover also caveats.

@TopperDEL
Copy link
Contributor Author

Thanks @mniewrzal - I'm not sure if I completely get what you mean. I don't know what "scope" is in this case and how it interacts with a "caveat".

Is there something I can do? Will your PR #3689 render my restrict_api_key from this PR obsolete?

@mniewrzal
Copy link
Contributor

Scope is a concept that is not exposed in uplinkc yet. Its keeping together satellite address, api key and encryption access. You can find it if you are using uplink cmd command by executing uplink share.

PR #3689 won't be adding restrict_scope, it should be separate PR. Regarding your help we have two ways: you can try making new version of restrict_scope (with my help) or wait for PR and review/test it on your side. New restrict_scope will be almost copy of your PR but with caveats and we also need unit test to verify it works as expected.

@TopperDEL
Copy link
Contributor Author

@mniewrzal Thanks for clarification! Would love to help on this one but this would need some further insights from you. Might we schedule a call again for Friday (again around 9 o'clock (GMT) which should be 8 o'clock (UTC))?

@mniewrzal
Copy link
Contributor

@mniewrzal Thanks for clarification! Would love to help on this one but this would need some further insights from you. Might we schedule a call again for Friday (again around 9 o'clock (GMT) which should be 8 o'clock (UTC))?

I'm not sure if I will be available then but ping me 30 min earlier with email and I will confirm.

@TopperDEL
Copy link
Contributor Author

This PR is not necessary anymore, as PR #3724 introduces the new restrict_scope-function.

Therefore I'm closing this one.

@TopperDEL TopperDEL closed this Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants