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

Support for https endpoints in witnesses and controllers #555

Merged
merged 3 commits into from
Aug 20, 2023
Merged

Support for https endpoints in witnesses and controllers #555

merged 3 commits into from
Aug 20, 2023

Conversation

rodolfomiranda
Copy link
Contributor

No description provided.

@AlexAndrei98
Copy link
Contributor

tests are passing!

@jasoncolburne
Copy link
Contributor

@jasoncolburne
Copy link
Contributor

jasoncolburne commented Aug 11, 2023

https://github.com/WebOfTrust/keripy/pull/555/files#diff-a4229d5895c922898cdeb03b5bb5c550ea2bf933cef6772027ffa763936ca630R734 here too, though I'm less sure here - you may need a new class? I think if you did, it would be very similar and probably a small refactor

@rodolfomiranda
Copy link
Contributor Author

looks like it may need mods

Thanks for the catch. I added the change there and in other places as well.

here too, though I'm less sure here - you may need a new class? I think if you did, it would be very similar and probably a small refactor

Changed there too. I don't think a new class is needed. Everything is handled by the same http.clienting.Client with the proper initialization.

@rodolfomiranda
Copy link
Contributor Author

oops, tests failing. Fixing...

@pfeairheller
Copy link
Member

I'm afraid we need a more comprehensive solution to this. Just defaulting to either HTTP or HTTPs may cause problems in the future. Now that we want to support. HTTPs, I think we should allow for it to fail if only HTTP is available which this code will not do.

We probably need to figure out how to make this a parameter passed into the methods for querying or publishing to a witness.

@rodolfomiranda
Copy link
Contributor Author

I'm afraid we need a more comprehensive solution to this. Just defaulting to either HTTP or HTTPs may cause problems in the future. Now that we want to support. HTTPs, I think we should allow for it to fail if only HTTP is available which this code will not do.

We probably need to figure out how to make this a parameter passed into the methods for querying or publishing to a witness.

isn't the scheme in the OOBI resolution such parameter?

@pfeairheller
Copy link
Member

I'm afraid we need a more comprehensive solution to this. Just defaulting to either HTTP or HTTPs may cause problems in the future. Now that we want to support. HTTPs, I think we should allow for it to fail if only HTTP is available which this code will not do.
We probably need to figure out how to make this a parameter passed into the methods for querying or publishing to a witness.

isn't the scheme in the OOBI resolution such parameter?

No. This is about endpoint URLs, not the OOBIs. They are unrelated.

@pfeairheller
Copy link
Member

It doesn't matter what scheme is used for an OOBI, the resolution of that OOBI can return endpoints with any scheme they want. The controller may only want to use witnesses that expose HTTPs. With this code, the user could end up using witnesses with HTTP and not even be aware.

@rodolfomiranda
Copy link
Contributor Author

It doesn't matter what scheme is used for an OOBI, the resolution of that OOBI can return endpoints with any scheme they want. The controller may only want to use witnesses that expose HTTPs. With this code, the user could end up using witnesses with HTTP and not even be aware.

got the point. This patch definitively does not solve that problem. It only allows https as an alternative to http endpoints.

@pfeairheller
Copy link
Member

Decision made in last keri-dev community meeting to merge this PR as is and open a new issue for more enhanced support of HTTPs allowing the controller initiating a connection to specify the schemes that they want to use.

@pfeairheller pfeairheller merged commit a017e87 into WebOfTrust:development Aug 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants