Add functionality to authenticate via TCPIP

[trappitsch]

This was first mentioned in #386. The Yokogawa 6370 authentication via TCPIP can go with username, password, or with anonymous user identification, where the username anonymous is sent and any password is valid.

I added a private _authenticate routine to instrument and to the Yokogawa itself (where it is actually implemented). The authentication will likely be slightly different for different instruments, therefore it is specific to the instrument.
@slagroommarino it would be great if you can test this with the actual instrument!

The one thing that might be iffy: the password authentication is in plain text. While the instrument accepts MD5 hashing, the way it would have to be implemented requires user interaction, see manual page 3-8. Instead of a password, a challenge string is requested after sending the username, that challenge string together with the password would have to be MD5 hashed (hashlib could be used), and then sent back to the instrument. The password is not stored in ik, however, even disassociated will remain in memory. This is hard to get rid off I think without an actual user interaction.

What we could do, instead of sending the password in plain text via TCPIP is to do the MD5 hashing in ik, in this case inside the _authenticate in the instrument. What do you think @scasagrande? The manual is unfortunately not very clear on how things are hashed together, my best guess would be the following routine inside the instrument (and import hashlib) at the start:

    def _authenticate(self, username, password):
        # authenticate username, password are set
        _ = self.query(f'open "{username}"')
        resp = self.query("AUTHENTICATE CRAM-MD5 OK")
        # hash it
        pwd = hashlib.md5()
        pwd.update(resp)
        pwd.update(password)
        resp = self.query(pwd.hexdigest())
        if "ready" not in resp.lower():
            raise ConnectionError("Could not authenticate with username / password")

However, since the manual is not very clear, I wonder if @slagroommarino could test this before implementation.

With the discussion, I mark this as a draft PR for now.

[codecov]

Codecov Report

Merging #389 (12c9e00) into main (207131b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #389   +/-   ##
=======================================
  Coverage   99.03%   99.03%           
=======================================
  Files          88       88           
  Lines        8977     8994   +17     
=======================================
+ Hits         8890     8907   +17     
  Misses         87       87           

Flag	Coverage Δ
unittests	99.03% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/instruments/abstract_instruments/instrument.py	100.00% <100.00%> (ø)
src/instruments/yokogawa/yokogawa6370.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sbor23]

Just an idea, maybe it would be useful to adopt the approach requests takes:
an auth= keyword argument which can hold different things.
For example

• auth=("user", "password")
• some custom auth

https://requests.readthedocs.io/en/latest/user/authentication/

Some class-based auth is probably overkill for the moment, but using a single argument would allow flexibility in the future (for example if API keys could be used, or reading from a file).

[trappitsch]

Thanks for the comment, I quite like the idea. That would make it much more flexible and the auth keyword could hold different authentication arguments for different instruments, i.e., whatever is required. Will implement that.

[trappitsch]

@slagroommarino: If you have time to test (would still be great), you would now supply the username, password as:

osa = instruments.yokogawa.Yokogawa6370.open_tcip(host="IP_ADDRESS", port="PORT", auth=("USER", "PWD"))

Here's the updated version for encrypted authentication, see first comment for details on how to use this. Thanks already in advance!

def _authenticate(self, auth):
    username, password = auth
    _ = self.query(f'open "{username}"')
    resp = self.query("AUTHENTICATE CRAM-MD5 OK")
    # hash it
    pwd = hashlib.md5()
    pwd.update(resp)
    pwd.update(password)
    resp = self.query(pwd.hexdigest())
    if "ready" not in resp.lower():
        raise ConnectionError("Could not authenticate with username / password")

@scasagrande: Codecov upload seems to be failing...

[slagroommarino]

Thanks for the reminder, I'll try to test it somewhere next week!

[slagroommarino]

I managed to get hold of the OSA this morning, but unfortunately I can not get this to work. As a debug message, I get

TypeError: query() missing 1 required positional argument: 'cmd'

which happens on line 55, when it tries to query the username:

_ = self.query(cmd)

It seems that I cannot send any command or query from the auth function.
I'm not sure if that is helpful to you or not, but I have no idea where to look for the solution. If you need more info, let me know.

[trappitsch]

Not sure I understand the error message you get, however, it looks like I should have written in line 55:

_ = self.query(f'OPEN "{username}"')

but didnt capitalize the OPEN part. I assume that is the same line 55 in the yokogawa6370.py that you are talking about? Thanks for testing this!

[slagroommarino]

I was trying to figure out why the debugger does not give so much information, as I could not get into the query function itself whilst debugging. When I explicitly add the OSA instance in the parameters:

_ = self.query(self, f'OPEN "{username}"')

it does not give the error I described before, but it starts complaining about self not having a _file attribute. Could it be because the _authenticate function is executed before the __init__()?

[trappitsch]

Wow, the tests did not catch that at all... weird. I could reproduce the error with just a listening socket. Now __init__ is hit before any authentication takes place, so it should all work (or at least fail somewhere else ;))

[slagroommarino]

Progress! It just fails a little later though as it mentions that the Yokogawa object has no _prompt attribute. The debugger points at the function below which is in instrument.py. The return statement is the point at which the script fails with the aforementioned problem.

    @property
    def prompt(self):
        """
        Gets/sets the prompt used for communication.

        The prompt refers to a character that is sent back from the instrument
        after it has finished processing your last command. Typically this is
        used to indicate to an end-user that the device is ready for input when
        connected to a serial-terminal interface.

        In IK, the prompt is specified that that it (and its associated
        termination character) are read in. The value read in from the device
        is also checked against the stored prompt value to make sure that
        everything is still in sync.

        :type: `str`
        """
        return self._prompt

It was a quick test, hopefully it is helpful. But may be I can spend a bit more time on it tomorrow.

[trappitsch]

Makes sense, prompt was not initialized since I put the authentication into the wrong place. My "test server" didn't answer, so It just waited in the query loop and never reached the self.prompt...

I had to move the authentication out of instrument and put it into yokogawa6370 itself, since even if I would have put it into the right place, the terminator for the connection would still be wrong.

One more time, I think this should work for you. One test is currently failing, will fix that in a bit :) Thanks for your patience!

[slagroommarino]

The real-life test is not failing, I managed to download the spectrum! Thanks for taking the time to implement this 😄

[trappitsch]

This is great news, thanks for testing @slagroommarino. If I may bother you a bit further: now that this works I incorporated the MD5 hashing of the password such that we don't end up sending a plaintext password over TCP/IP. Only the _authenticate routine inside the yokogawa class changed, so the flow should still work :) Would you mind giving this another run?

Tests are currently failing, that's because I only changed the routine for now and will adopt the tests if this actually works (also gotta run in a couple of minutes ;))

Thanks!

[scasagrande]

This is looking great!!

[trappitsch]

Thanks @scasagrande, hope the MD5 hashed version works too :)
On that note, wanted to ping @slagroommarino briefly to see if you had any chance to check this edit. Thanks again for all your testing and help!

[slagroommarino]

I'll try tomorrow morning, as the machine is in heavy use at the moment :)

[trappitsch]

thank you!

[slagroommarino]

Alright, there seems to be a connection as the machine's 'remote' indicator is on, but it does not continue with the rest of the commands. When I close the connection from the machine, the script stops with the error: Socket connection timed out before reading a termination character.

Removing the hash fixes the problem, but that is of course not a solution :P

Btw, the coming three weeks I'm not in the office, so next time testing will be a while.

[trappitsch]

Sorry, I was out myself for a week. This is very strange, I don't really know why this should be. It seems as if it forgets to send a terminator all of a sudden (or sends a wrong one?)
I added some print statements here and there, whenever you have time it would be great if you can run this and post the output that you get... Thanks so much!

[slagroommarino]

That is wild haha, never new that that was a thing. I can try tomorrow to add or change the user with an actual username and password ;)

[trappitsch]

Indeed, quite the fancy instrument you got there. Both should hopefully work now, the anonymous user as well as any other user/password combination, the latter being submitted hashed. Fingers crossed...

[slagroommarino]

Alright, the test works with user 'anonymous', but fails for every other user I try. Here the output:

CMD: OPEN "anonymou"
REC: b'AUTHENTICATE CRAM-MD5.\r\n'
RESP: AUTHENTICATE CRAM-MD5.
CMD: AUTHENTICATE CRAM-MD5 OK
REC: b'failed\r\n'
RESP: failed
CMD: 169ba01fa7db96c8b4f94b713a188c54

The script ends with a socket timeout 'before reading a termination character'.

[trappitsch]

Weird. One last ditch effort, looking at some other code I might have missed sending the " around the request for MD5 authentication. Not super clear from the manual, maybe it works. Could you try:

• authentication as given user that is not "anonymous"
• test with the "anonymous" user to see if that still works (if the first one succeeded)

From the output, it seems like the request to send a challenge fails and it just returns failed, which is obviously not the challenge ;)
Thanks!

[trappitsch]

Any chance to give this one a run @slagroommarino? Sorry to bother you and no worries if you don't have time at the moment. Really appreciate your help with this!

[slagroommarino]

Yes busy times ;)

Unfortunately, the result has not changed. The debugger stops at line 78 in the yokogawa6370.py:

73        # hash it
74        if username.lower() != "anonymous":  # so we need an actual password
75            pwd = hashlib.md5()
76            pwd.update(bytes(resp, "utf-8"))
77            pwd.update(bytes(password, "utf-8"))
78            resp = self.query(pwd.hexdigest())

[trappitsch]

Thanks for testing and sorry for the long delay. I don't understand why this is failing anymore. Re-reading the manual results in no further insight of what might be going on here...
I guess at this point, in order to get this in and make the authentication working, we can simply revert to before and send the password as plain text/unhashed to the device. This should work with the anonymous user as well as with any user. I prepared a new version where I took the hashing out, if you can, would be fun to test this one last time.

@scasagrande: What do you think? The authentication is now implemented as an option for any device and hashing - if wanted and functioning - can be implemented for other devices on a per-device level...

[scasagrande]

Well most of this PR is specific to the Yokogawa 6370. I'm fine with us starting with just sending it with plain text to get something functioning.

[trappitsch]

Okay, tests are passing now, @slagroommarino if you have time to test this once more (sorry for the hassle and thanks for all your checks and time!) then I'd be ready to give this for review :)

[trappitsch]

Just checking in: Did you have any chance to have a look at this @slagroommarino?

[trappitsch]

There seems to be no time for testing with the hardware... Since (1) the unittests are functional and (2) this has been tested before thanks to @slagroommarino, I mark this ready for review.

[Neustradamus]

Hello all,

I see "CRAM-MD5", but it is not secure :/

About old unsecure mechanisms and it is not new:

• https://tools.ietf.org/html/draft-ietf-sasl-crammd5-to-historic-00
• https://tools.ietf.org/html/draft-zeilenga-luis140219-crammd5-to-historic-00
• https://tools.ietf.org/html/rfc6331

[trappitsch]

@scasagrande what do you think of this?

[scasagrande]

seems fine other than my one comment

[scasagrande]

remove me

[trappitsch]

Well, it's been a while. How are things looking @scasagrande? Should have incorporated the changes. Mostly asking because we could start testing for 3.12 and drop 3.7... (of course not in this PR) 😄

[scasagrande]

Whoops!