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

[Python] Support for HTTP signature #4958

Merged
merged 128 commits into from
Jan 27, 2020

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented Jan 10, 2020

This is adding a new "HTTP signature" security schemes (https://datatracker.ietf.org/doc/draft-cavage-http-signatures/)? HTTP signatures is still a IETF draft, and hopefully it will become an official RFC this year. On one hand one may argue it shouldn’t be added because it’s still a draft, but on the other hand it is already being used by multiple products, so there may be benefits to support it. It is possible multiple organizations are independently adding the same security scheme.

Ideally one way to address the problem would be to make it possible to add new security schemes (such as HTTP signature) without being required to fork the OpenAPITools repo. But given the current code structure, it’s not clear to me how this could be achieved. Adding new security schemes involves surgery in multiple locations (Java codegen, templates, mustache tags).

I raised this point in Slack before opening the PR.

This PR depends on #4993 and #4992.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@spacether
Copy link
Contributor

@sebastien-rosset can you provide some extra context around this PR?
There is no description up above.
When I look in the spec: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#securitySchemeObject
I don't see the signature scheme that you are using.
Is there a draft proposal to add that to OpenApi, where?

@spacether
Copy link
Contributor

spacether commented Jan 13, 2020

This looks really neat. I have a couple of points.

  • Could we break out the java code adding the HTTP signature addition to a separate PR?
    • That way your go.go-experimental/pytheon-experimental PRs could depend upon it and be more stand-alone in what they are adding
    • Also, it makes this feature a lot easier to understand to see a clear origin PR
  • Could you add more testing to this? I am interested in verifying that a payload is what you expect with a specific signature type.
  • What do you think about us separating creating the python-experimental openapi sample into a separate PR then landing this update on it?
    • I also want to add tests of oneOf/anyOf to that sample, but us doing both at once like you have done is a huge number of files to review and makes our review work more difficult

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented Jan 13, 2020

This looks really neat. I have a couple fo points.

  • Could we break out the HTTP signature addition to a separate PR?

Do you mean codegen versus language specific (Python) changes?
I have opened #4993 for codegen.

  • That way your go.go-experimental/pytheon-experimental PRs could depend upon it and be more stand-alone in what they are adding
  • Could you add more testing to this? I am interested in verifying that a payload is what you expect with a specific signature type.
  • What do you think about us separating the python-experimental openapi sample into a separate PR then landing this update on it?

I have opened #4992 for the openapiv3 Python experimental samples

  • I also want to add tests of oneOf/anyOf to that sample, but us doing both at once like you have done is a huge number of files to review and makes our review work more difficult

makes sense, will do.

@sebastien-rosset
Copy link
Contributor Author

@spacether
Copy link
Contributor

Do you mean codegen versus language specific (Python) changes?
I have opened #4993 for codegen.

Yes, the codegen vs the language specific implementation. Thanks for opening that PR 👍

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented Jan 14, 2020

Because I have split the PR, and the python-experimental samples is now #4992, I think I need to wait for #4992 to be merged before I can add unit tests.
The unit tests will go under samples/openapi3/client/petstore/python-experimental/tests

@spacether
Copy link
Contributor

@sebastien-rosset can you resolve the merge conflict?

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented Jan 26, 2020

@sebastien-rosset can you resolve the merge conflict?

sure, working on it.
Update 1: done with the merge conflict. I also removed the call to .replace('+', '%20') in the signing.mustache file. This was accidentally introduced from a test implementation.

Update 2: The build has passed.

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented Jan 26, 2020

I looked at what would happen if there were unusual characters in the "path" or "query" segments of the URL, and how that would impact HTTP signature. There are some subtleties, so I opened OAI/OpenAPI-Specification#2119 and #5117. I think I may have to submit another PR to handle that situation, after we agree on what is the expected character encoding in the OAS spec.

@spacether spacether merged commit 4f350bc into OpenAPITools:master Jan 27, 2020
@jimschubert jimschubert added this to the 4.2.3 milestone Jan 27, 2020
@sebastien-rosset
Copy link
Contributor Author

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants