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

[CORE] Add support for HTTP signature #4993

Merged
merged 25 commits into from
Jan 26, 2020

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented Jan 13, 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.

I suggest merging #5049 before this PR.

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.

@sebastien-rosset
Copy link
Contributor Author

Copy/paste of discussion that occurred in Slack:

Jim Schubert
This is interesting, but there's no way to define this in OAS 3.0. Adding support to our project would be easy enough (via config options and DefaultCodegen implementation). However, I'd be hesitant to do this because there's other feature work and usability stuff to address which is part of OAS, and with this being in draft and no way to define in the spec I feel like any implementation would have to change soon (that adds maintenance and deprecation cycles).
I'd suggest opening an issue for tracking and for ideas from the community. I'd personally like the functionality, but it might be too early.

Sebastien Rosset 4 days ago
Technically, I would argue it's possible without any change to the OAS spec. Create a securityScheme with "type" set to "http", and "scheme" set to "http-signature". Well, almost... The "scheme" attribute in OAS is just a string, and it's supposed to be a value specified in the IANA registry: https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml. But at this point, the http-signature is not registered with IANA.

Sebastien Rosset 4 days ago
I will open an issue

Jim Schubert 4 days ago
I see, I thought your suggestion was to add a new type + scheme (http-signature + Signature) which wouldn't be supported because type is constrained by the spec. With type=http and scheme=signature, we'd just need to define our supported vendor extensions for options like keyId, algorithm or any other options from the draft. Again, I think it's doable without much modification because it would be done in DefaultCodegen or maybe ModelUtils (to support the vendor extensions) and probably some helper properties added to CodegenSecurity. Then, the new feature can be added to Generators iteratively as the community requests support.

Jim Schubert 4 days ago
Let me know if you're interested to take a stab at it and need any assistance. The python link didn't work, so I'm not sure how far you've gotten with implementation.

Sebastien Rosset 4 days ago
ok, sure, sounds good. I do have a working, tested implementation for golang. For Python, I also have code, but I have not tested it. I'm interested to upstream this capability because it could benefit the community and also quite frankly it's a pain to maintain a fork of the many OSS repos we use.

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented Jan 14, 2020

There is a build failure because of unrelated PHP uncommitted file:

UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'
Perform git diff
diff --git a/samples/server/petstore/php-symfony/SymfonyBundle-php/Api/PetApiInterface.php b/samples/server/petstore/php-symfony/SymfonyBundle-php/Api/PetApiInterface.php
index 3d822e56f2..864fa5cd80 100644
--- a/samples/server/petstore/php-symfony/SymfonyBundle-php/Api/PetApiInterface.php
+++ b/samples/server/petstore/php-symfony/SymfonyBundle-php/Api/PetApiInterface.php

This requires changes from #5001

@richardwhiuk richardwhiuk requested review from jimschubert and removed request for richardwhiuk January 16, 2020 12:18
@sebastien-rosset
Copy link
Contributor Author

Initially I added the new 'http signature' auth scheme to the existing petstore-with-fake-endpoints-models-for-testing.yaml. But then after running the scripts to generate PHP code, I hit a new issue that I have opened: #5025.

IMO, all generators should have the following logic: if the security scheme type is "http", then inspect the value of the "scheme" attribute. If the "scheme" is "basic", then generate the code for http basic authentication. If the value of the "scheme" is unknown to this particular generator, then don't generate anything.

As a workaround, I have copied a new YAML file modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml. Its content is the same as modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml, with a few lines changed to specify HTTP signature. That file can be used for the python and golang generators. Then when #5025 is fixed, we can remove the file.

@sebastien-rosset
Copy link
Contributor Author

The diff in samples/openapi3/client/petstore/go-experimental/go-petstore/api/openapi.yaml seem to be completely unrelated.

@jimschubert jimschubert added this to the 4.2.3 milestone Jan 26, 2020
@jimschubert jimschubert merged commit 20afa87 into OpenAPITools:master Jan 26, 2020
@jimschubert
Copy link
Member

Thanks for this work!

@sebastien-rosset sebastien-rosset deleted the http-signature branch May 23, 2020 19:24
tiffmaelite added a commit to tiffmaelite/openapi-generator that referenced this pull request Apr 14, 2023
the signature scheme was added by OpenAPITools#6058 for jersey2-experimental (now jersey2-java8) only but this change affected all samples; and it was not removed when there is another special yaml file with http-signature-test (OpenAPITools#4993) was introduced
tiffmaelite added a commit to tiffmaelite/openapi-generator that referenced this pull request Apr 14, 2023
the signature scheme was added by OpenAPITools#6058 for jersey2-experimental (now jersey2-java8) only but this change affected all samples; and it was not removed when there is another special yaml file with http-signature-test (OpenAPITools#4993) was introduced
tiffmaelite added a commit to tiffmaelite/openapi-generator that referenced this pull request Apr 17, 2023
+ remove http signature from test input yaml when not supported
+ use http-signature file for python-nextgen and python-legacy instead of standard test input file

the signature scheme was added by OpenAPITools#6058 for jersey2-experimental (now jersey2-java8) only but this change affected all samples; and it was not removed when there is another special yaml file with http-signature-test (OpenAPITools#4993) was introduced
tiffmaelite added a commit to tiffmaelite/openapi-generator that referenced this pull request May 22, 2023
+ remove http signature from test input yaml when not supported
+ use http-signature file for python-nextgen and python-legacy instead of standard test input file

the signature scheme was added by OpenAPITools#6058 for jersey2-experimental (now jersey2-java8) only but this change affected all samples; and it was not removed when there is another special yaml file with http-signature-test (OpenAPITools#4993) was introduced
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.

2 participants