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

feat: added TLS encryption support to API calls (HTTPS) #2658

Closed
wants to merge 2 commits into from

Conversation

eravellaSC
Copy link

Solving #2491, I added support for TLS-encryption for the HTTP API calls. The gin server was already set up to do so, the only thing missing were the conf paths.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #2658 (062e3a2) into main (5dae270) will decrease coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2658      +/-   ##
==========================================
- Coverage   59.41%   59.31%   -0.10%     
==========================================
  Files         144      144              
  Lines       15238    15238              
==========================================
- Hits         9053     9038      -15     
- Misses       5549     5560      +11     
- Partials      636      640       +4     
Files Coverage Δ
internal/conf/conf.go 65.17% <ø> (ø)
internal/core/api.go 66.29% <100.00%> (ø)

... and 5 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@eravellaSC
Copy link
Author

I double checked the failing tests and coverage reports and they mention files and checks that I have not touched. It's a very small modification, and the codebase was already ready to accomodate this simple addon. Is the coverage slowing down the review process? I can dig deeper if it's needed.

@eravellaSC
Copy link
Author

I am sorry to bother directly, @aler9, but I'd like to know if there is interest in adding this feature. If not I can close the PR.

@aler9
Copy link
Member

aler9 commented Jan 8, 2024

Hello @eravellaSC, of course there's interest, but the problem is that currently the API is missing a strong authentication system, therefore it's meant to be used either in a closed environment or behind a reverse proxy which provides authentication and TLS termination.

If i merge this feature without providing authentication, there's risk of giving the impression that the API is safe to use.

Furthermore, nowadays valid TLS certificates are generated by authorities that periodically query the corresponding domain through HTTP, and that would mean that some users may try to expose the API publicly. Since it's easy to scan the internet and detect running instances of MediaMTX, this would mean creating a Botnet.

Anyway, the authentication issue will be solved soon.

@eravellaSC
Copy link
Author

@aler9 Thanks for the update, I didn't think of that in that way. I'll wait for the auth to be merged then.

@aler9
Copy link
Member

aler9 commented Apr 21, 2024

replaced by #3280

@aler9 aler9 closed this Apr 21, 2024
aler9 added a commit that referenced this pull request Apr 21, 2024
aler9 added a commit that referenced this pull request Apr 21, 2024
aler9 added a commit that referenced this pull request Apr 21, 2024
Copy link
Contributor

This issue is mentioned in release v1.8.0 🚀
Check out the entire changelog by clicking here

@eravellaSC eravellaSC deleted the feature/HTTPS branch May 2, 2024 06:43
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.

None yet

2 participants