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

Code to query the IP address data for our WiFi interface. #206

Closed
wants to merge 43 commits into from

Conversation

dannybackx
Copy link
Contributor

This allows the microcontroller to query e.g. the IP address of the IoT
device and communicate it to trusted parties.

This allows the microcontroller to query e.g. the IP address of the IoT
device and communicate it to trusted parties.
This basically parameterizes the serial bridges to ports 23 and 2323.
@fuzzball03
Copy link

Looks like most of the code to this point modifies serial bridge settings. (Might be what I need to address an odd issue I ran into with my pull request #205)

I don't see how this sends the IP to other hosts. Am I missing something here?

@dannybackx
Copy link
Contributor Author

On 10/30/2016 04:05 PM, Alex wrote:

Looks like most of the code to this point modifies serial bridge settings. (Might be what I need to address an odd issue I ran into with my pull request #205 #205)

I don't see how this sends the IP to other hosts. Am I missing something here?

Alex,

My code submission consists of two pieces :

  • the one you're responding to is the one running on the ESP. The cmdGetWifiInfo() will send IP address, network mask, and gateway address of the ESP to the requester; see lines 202 .. 204.
  • The accompanying patch is for el-client (see Query IP address info from the application el-client#25). It adds a function that actually does the query.

An Arduino example sketch for this is in http://danny.backx.info/danny/backup/test1.ino . I should add this to the ELClient/examples tree I guess.

Output of this sample sketch (with debug off in the library code) is :

ELClient test
EL-Client synced!
WIFI CONNECTED
IP Address 192.168.1.149
Network mask 255.255.255.0
IP Gateway 192.168.1.1

 Danny

Danny Backx - danny.backx@scarlet.be - http://danny.backx.info

@dannybackx
Copy link
Contributor Author

On 10/30/2016 05:15 PM, Danny Backx wrote:

On 10/30/2016 04:05 PM, Alex wrote:

Looks like most of the code to this point modifies serial bridge settings. (Might be what I need to address an odd issue I ran into with my pull request #205 #205)

Replying to my own mail means I hit the Send button too quickly ;-)

I only answered your second question and didn't look deep enough into the link you sent.

I am indeed also working on security, it looks like that'll fix issue #167 .
Currently working on the web user interface to set which ports to enable (overriding 23 and 2323).

My security issue is that I want you to be disable the ports, hide the access at another port number, or password-protect them.

 Danny

Danny Backx - danny.backx@scarlet.be - http://danny.backx.info

@fuzzball03
Copy link

I already created a pull request with web interface to choose the ports.
Take a look there and feel free to comment.
Pull #205

@fuzzball03
Copy link

Nice work on everything else sir - I don't see any obvious issues.
Reading over your code certainly helped me learn and understand more about this and C devlopment.

@dannybackx
Copy link
Contributor Author

On 10/30/2016 06:23 PM, Alex wrote:

I already created a pull request with web interface to choose the ports.
Take a look there and feel free to comment.
Pull #205 #205

and

Nice work on everything else sir - I don't see any obvious issues.
Reading over your code certainly helped me learn and understand more about this and C devlopment.

Thanks for pointing me to our parallel development.

I found myself writing and debugging javascript for the first time so I'll look into your code, it may be more efficient to continue my efforts based on yours. Or something like that :-)

 Danny

Danny Backx - danny.backx@scarlet.be - http://danny.backx.info

Copy link
Member

@tve tve left a comment

Choose a reason for hiding this comment

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

Can you describe somewhere what the security changes are about? I don't think I understand what you are trying to accomplish.

Any chance you could split your PR in two?

Makefile Outdated
#
# Default settings for access over TCP/IP connections
#
# Modes are 0 (unsecure), 1 (disabled), 2 (secure)
Copy link
Member

Choose a reason for hiding this comment

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

s/unsecure/insecure/

Makefile Outdated
@@ -99,6 +99,15 @@ MCU_ISP_PIN ?= 13
LED_CONN_PIN ?= 0
# GPIO pin used for "serial activity" LED, active low
LED_SERIAL_PIN ?= 14
#
# Default settings for access over TCP/IP connections
Copy link
Member

Choose a reason for hiding this comment

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

HTTP uses TCP/IP, did you mean "telnet"?

Makefile Outdated
PORT1_MODE ?= 0
PORT1_PORTNUMBER ?= 23
PORT2_MODE ?= 0
PORT2_PORTNUMBER ?= 2323
Copy link
Member

Choose a reason for hiding this comment

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

There is a PR #205 that going to get merged to change the ports via the web UI. I'd rather not have additional settings in the Makefile too. It gets very confusing when there are settings in the Makefile 'cause it's not obvious at all which take effect when. There are settings for Wifi in the Makefile and that has been a confusing mess, and those are necessary because without wifi it's hard to connect after flashing, so at least there's a good reason for them.

cmd/cmd.h Outdated
@@ -58,6 +58,8 @@ typedef enum {
CMD_SOCKET_SETUP = 40, // set-up callbacks
CMD_SOCKET_SEND, // send data over UDP socket

CMD_GET_WIFI_INFO = 50,// Query IP address info
Copy link
Member

Choose a reason for hiding this comment

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

Please put this sequentially after CMD_GET_TIME, no need to start a new section for this.

cmd/handlers.c Outdated
cmdResponseBody(&info.gw.addr, sizeof(info.gw.addr));
cmdResponseEnd();

return;
Copy link
Member

Choose a reason for hiding this comment

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

A return stmt is not needed here.

CmdRequest req;

cmdRequest(&req, cmd);
if(cmd->argc != 0 || cmd->value == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Mhh, I have to look what has been done elsewhere, but I'm not sure why this should return anything if no callback has been passed.

uint32_t callback = req.cmd->value;

struct ip_info info;
wifi_get_ip_info(0, &info);
Copy link
Member

Choose a reason for hiding this comment

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

This gets the STA info. Shouldn't this thing return both the STA and AP info? Or provide two calls for the two?

Copy link
Member

Choose a reason for hiding this comment

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

I think there have also been requests to get the MAC address, maybe that easy to include as well.

@@ -54,6 +54,8 @@ static char *wifiReasons[] = {
static char *wifiMode[] = { 0, "STA", "AP", "AP+STA" };
static char *wifiPhy[] = { 0, "11b", "11g", "11n" };

static char *portMode[] = { "unsecure", "disabled", "secure" };
Copy link
Member

Choose a reason for hiding this comment

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

s/unsecure/insecure/

@dannybackx
Copy link
Contributor Author

dannybackx commented Oct 30, 2016

Guys,

I'm getting confused, maybe you are as well.

I thought the point where I issued a pull request indicated the work I finished (unreviewed but a working prototype) for one functionality.
I also had another functionality in mind so I continued to work on that today.

And now it seems we're all looking at my work in progress which obviously is a mess.

Goal #1 I had was to be able to identify the IoT device even if it's on another network than mine.
The thing I asked for, and then wrote, is a way to query the IP address of the device from Arduino code, so I can have the device
share that info with trusted parties.
Goal #2 was to secure the device.
Obvious hackable entries are ports 23 and 2323, this is what I started working on.
As I've just discovered, this overlaps with Alex's work so let's see how to move this ahead together rather than apart.

My idea (mentioned on the chat this weekend, no replies) was to be able to password protect and/or disable the 23 and 2323 accesses.
While at it, I was also creating a way to overrule the port numbers which is what Alex was doing.

Securing the web interface is also something that comes to mind. Haven't acted on this yet.

Comments welcomed :-)

 Danny

@dannybackx
Copy link
Contributor Author

I'll clean up this mess tomorrow. Leave only the stuff that belongs here, and make changes based on @tve 's review.

@fuzzball03
Copy link

I follow you sir. Enjoying my Sunday, so ill go over my stuff tomorrow as
well. Ill probably restructure a bit based on what you've done.
Nice to see steady contributions Danny - thank you.
Thanks goes to @tve as well.

On Sun, Oct 30, 2016, 4:23 PM dannybackx notifications@github.com wrote:

I'll clean up this mess tomorrow. Leave only the stuff that belongs here,
and make changes based on @tve https://github.com/tve 's review.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#206 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFimNPxqDsnaR64J5VvPXClRt2nfe4pSks5q5QrbgaJpZM4KkUxt
.

@tve
Copy link
Member

tve commented Oct 30, 2016

Hey, no rush, this is supposed to be fun. Take your time, nothing is running away ;-)

@dannybackx
Copy link
Contributor Author

On 10/31/2016 12:56 AM, Thorsten von Eicken wrote:

Hey, no rush, this is supposed to be fun. Take your time, nothing is running away ;-)

Well, that's good because I don't know very well how to proceed.

I'll try to roll back all the commits I did after the work I wanted to share. (So I'll leave only the stuff that matches issue #206.)

The next question is how I can proceed with the other work. Given Alex's userid, I guessed I could find his work at github.com/fuzzball03/esp-link so I just checked out that code base to my disk. I don't know how we can use github to work together on this. Should e.g. Alex give me permissions on his fork so we make a joint submission ?

I see an additional field down in the Home window (home.html changes). I think the C language source code changes I supplied can implement that user interface.

I have no problem admitting I've not done any javascript work before, so I'm really learning there. I have a lot of experience with C though.
It looks like Alex's skills may be the other way around. Can we divide the work ?

Also what do we want to implement ?

As I wrote earlier I want to secure my device before putting it in the wild. That's not Alex's goal. Thorsten, are you the one saying which functionality we implement ?

Thanks,

 Danny

Danny Backx - danny.backx@scarlet.be - http://danny.backx.info

@fuzzball03
Copy link

@dannybackx ill be happy to give you write permission to my repo.

You're spot on - C is far from a strength of mine. I've never done anything C consisting of more than 3 headers till this project. Give me java, html, or php any day. I'm certainly not use to trying to manage or reduce memory/ram usage.

I believe focusing work on one change per pull would be easier for @tve and everyone else. Not to say you're not welcome to use my repo to implement your planned security features.

Quick question for you Danny - what's your reasoning for placing telnet/serial bridge settings in the Wi-Fi page?
Perhaps I should create a new page all together for Telnet/Serial bridge settings(which will easily incorporate the security settings you've been working on).

@dannybackx
Copy link
Contributor Author

Cleaned up now, I think.
I've done a quick implementation to show that MAC info can indeed be added easily.
Obviously this is not the right way to do it. Either the getMac() method needs to be merged in getWifiInfo() or the calls need to be separated in esp-link as well.

@dannybackx
Copy link
Contributor Author

On 11/01/2016 01:14 AM, Alex wrote:

@dannybackx https://github.com/Dannybackx ill be happy to give you write permission to my repo.

You're spot on - C is far from a strength of mine. I've never done anything C consisting of more than 3 headers till this project. Give me java, html, or php any day. I'm certainly not use to trying to manage or reduce memory/ram usage.

I believe focusing work on one change per pull would be easier for @tve https://github.com/tve and everyone else. Not to say you're not welcome to use my repo to implement your planned security features.

Quick question for you Danny - what's your reasoning for placing telnet/serial bridge settings in the Wi-Fi page?
Perhaps I should create a new page all together for Telnet/Serial bridge settings(which will easily incorporate the security settings you've been working on).

I'm happy with discussing what and how to implement for a few days before actually doing it.

After that, we'll also know which changes to combine, or not, into pull requests.

You asked why I put the serial bridge security stuff on the "WiFi Station" page. That's because I found that it belonged with the "Special Settings" on that page. But you can follow similar arguments to put it on the same page as "Pin assignment" which is on the Home page.
There appears to be some duplication there (System overview on Home has almost the same content as WiFi State on the Wifi Station page).

Should the duplication remain ?

 Danny

Danny Backx - danny.backx@scarlet.be - http://danny.backx.info

Increase one variable's size to cope with program size larger than 64K
Bugfix in query firewall external address.
Copy link
Member

@tve tve left a comment

Choose a reason for hiding this comment

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

Thanks! But yikes! this is a ton of stuff! Any chance to break it up into reviewable pieces?

# -DLED_CONN_PIN=$(LED_CONN_PIN) -DLED_SERIAL_PIN=$(LED_SERIAL_PIN) \
# -DVERSION="$(VERSION)"
#
CFLAGS += -Os -ggdb -std=c99 -Wpointer-arith -Wundef -Wall -Wl,-EL -fno-inline-functions \
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove -Werror?

#SDK_BASE := $(wildcard $(XTENSA_TOOLS_ROOT)/../../$(SDK_VERS))
#endif
ifeq ($(SDK_BASE),)
SDK_BASE := $(wildcard $(XTENSA_TOOLS_ROOT)/../../$(SDK_VERS))
Copy link
Member

Choose a reason for hiding this comment

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

Please don't uncomment in what you submit.

@@ -52,12 +52,15 @@ ESP_HOSTNAME ?= esp-link
# Base directory for the compiler. Needs a / at the end.
# Typically you'll install https://github.com/pfalcon/esp-open-sdk
# IMPORTANT: use esp-open-sdk `make STANDALONE=n`: the SDK bundled with esp-open-sdk will *not* work!
XTENSA_TOOLS_ROOT ?= $(abspath ../esp-open-sdk/xtensa-lx106-elf/bin)/
XTENSA_TOOLS_ROOT ?= $(abspath ../espressif/xtensa-lx106-elf/bin)/
Copy link
Member

Choose a reason for hiding this comment

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

please keep std...

@@ -387,6 +407,7 @@ $(FW_BASE)/user2.bin: $(USER2_OUT) $(FW_BASE)
$(Q) COMPILE=gcc PATH=$(XTENSA_TOOLS_ROOT):$(PATH) python $(APPGEN_TOOL) $(USER2_OUT) 2 $(ESP_FLASH_MODE) $(ESP_FLASH_FREQ_DIV) $(ESP_SPI_SIZE) 1 >/dev/null
$(Q) rm -f eagle.app.v6.*.bin
$(Q) mv eagle.app.flash.bin $@
@echo "** user2.bin uses $$(stat -c '%s' $@) bytes of" $(ESP_FLASH_MAX) "available"
Copy link
Member

Choose a reason for hiding this comment

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

user2 is the same size as user1, so this is just noise, isn't it?

@tve
Copy link
Member

tve commented Jun 16, 2017

I believe this is superseded by #289 and #290. Please feel free to open a fresh PR for anything else you'd like to get merged! Thanks!

@tve tve closed this Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants