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

mbed TLS client support, try 2 #48

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Conversation

tve
Copy link
Contributor

@tve tve commented Jun 25, 2019

This is a tentative PR to replace #43 to account for the significant restructuring that was made on master since that PR.
This still only has client-side TLS support.
More testing is required before this can be considered for merging, I wanted to get this queued-up in case someone else is also working on this to avoid duplication of effort.

src/AsyncTCP.cpp Outdated Show resolved Hide resolved
src/AsyncTCP.cpp Outdated
}
return ERR_BUF; // for lack of a better error value
}
return ERR_OK;
Copy link
Owner

Choose a reason for hiding this comment

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

you are in a while loop. should continue here maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/AsyncTCP.cpp Outdated Show resolved Hide resolved
@me-no-dev
Copy link
Owner

@fremouw left a few comments. Also! Very important. You will notice that all calls to LwIP's tcp_* api are done through callbacks executed on LwIP's thread. In the ssl C code I notice that calls to LwIP are being made directly. This will cause a slew of issues and can not be merged as is.

@me-no-dev
Copy link
Owner

@tve thanks for taking care of rebasing this :)

@robert-alfaro
Copy link
Contributor

I'm all for examples, but there are none in this repo... Why the requirement before merging?

@tve
Copy link
Contributor Author

tve commented Dec 22, 2019

Yup, that's where I stopped...
Also, the codacity issues are b.s. They complain about any use of strlen and offer zero alternatives. It doesn't make sense to convert all the code to use String...

@robert-alfaro
Copy link
Contributor

robert-alfaro commented Jan 3, 2020

Have you tried strnlen()? I think it's considered safe because you pass in the size (compared to strlen running until it finds a \0)

@nemidiy
Copy link

nemidiy commented Jan 11, 2020

Hey, great stuff @tve . I used this branch to test out TLS on https://github.com/marvinroger/async-mqtt-client and works great. I am also here to offer any help needed to merge this. I have one question to ask. I am getting constantly the following message when using this PR :

[E][AsyncTCP.cpp:1130] _poll(): 0x3ffcc15c != 0x3ffd1ccc

Besides the E, the stack behaves fine, so I am not sure why I get that.

Tried to look at the code but I am not understanding what's up.
https://github.com/tve/AsyncTCP/blob/mbed-tls-try2/src/AsyncTCP.cpp#L1130

Any ideas ? thanks.

@robert-alfaro
Copy link
Contributor

@tve may I have permission to push to your branch?

@tve
Copy link
Contributor Author

tve commented Jan 20, 2020

I've been busy elsewhere, need to get back to this... If you create a PR against my repo I'd be happy to pull it in. Just create a PR in github with tve/AsyncTCP : mbed-tls-try2 as base.

@robert-alfaro
Copy link
Contributor

I was trying to push straight to this PR branch..avoiding forking your forked repo.

@tve
Copy link
Contributor Author

tve commented Jan 20, 2020

You should not need to create a new fork, you simply select my fork & branch in the github dialog to create a PR.

@robert-alfaro
Copy link
Contributor

robert-alfaro commented Jan 20, 2020

Is this assuming I've forked the upstream repo? What I've done thus far is:

git clone git@github.com:me-no-dev/AsyncTCP.git
git fetch origin pull/48/head:pr/48
git checkout pr/48
<work>
<commit>
git push git@github.com:tve/AsyncTCP pr/48:mbed-tls-try2

and obviously the push will fail because permissions:

ERROR: Permission to tve/AsyncTCP.git denied to robert-alfaro.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

fwiw, changes so far are just change strlen to sizeof for pers[]. Then I could do the rebase/merge. The strlen for psk_ vars is the way it's done everywhere.

@tve
Copy link
Contributor Author

tve commented Jan 21, 2020

Yes, I assumed you had a github account and had forked. Got a git patch?

@robert-alfaro
Copy link
Contributor

I conflated this project with another..I do have a fork under my work email on same github account. I'll have something for you.

robert-alfaro and others added 3 commits January 21, 2020 16:51
* Fix LoadProhibited (me-no-dev#73)
* Use sizeof instead of strlen for const char[]
* Add Kconfig option to control ASYNC_TCP_SSL_ENABLED
* Optionally include ssl header files
* Add null check for psk_ident and pskey
* Do not default to PSK when root_ca is not explcitly set. tcp_ssl_new_client() has a case to handle this.
* Move psk null checks to top of function, remove unneeded include, syntax cleanup.

Authored-by: Bob <bob.mooij@gmail.com>
@robert-alfaro
Copy link
Contributor

robert-alfaro commented Feb 2, 2020

@me-no-dev bump

@paytah232
Copy link

paytah232 commented Apr 7, 2020

What's the best way for a github noob to get this into my download of ESPAsyncWebServer and make it work in arduino code? I'm currently using a combination of AsyncWebServer and AsyncWiFiManager, is it going to be a direct file overwrite? Is there a readme on how to implement? Cheers! @tve @me-no-dev

@fransking
Copy link

What's the best way for a github noob to get this into my download of ESPAsyncWebServer and make it work in arduino code? I'm currently using a combination of AsyncWebServer and AsyncWiFiManager, is it going to be a direct file overwrite? Is there a readme on how to implement? Cheers! @tve @me-no-dev

Adding this into platformio.ini worked for me

lib_deps =
AsyncTCP=https://github.com/tve/AsyncTCP/archive/mbed-tls-try2.zip
async-mqtt-client=https://github.com/tve/async-mqtt-client/archive/master.zip

@wachidsusilo
Copy link

is there any example to use this fork?

@roipoussiere
Copy link

What is the status of this, is this can be merged?

@MichaelUray
Copy link

What is there actually missing to merge these changes to add TLS client support?

@podaen
Copy link

podaen commented Oct 20, 2022

if we wait on this, it will never go through. It has been years now. There are not enough users on those platforms with the same interests!

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.