Opened 16 years ago

Closed 13 years ago

#1376 enhancement closed fixed (fixed)

hashed host entries in known_hosts for conch (SSH)

Reported by: count0 Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p, count0, ericf, Thijs Triemstra Branch: branches/hashed-1376
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description


Attachments (2)

review-part-1.txt (37.4 KB) - added by Jonathan Lange 13 years ago.
First part of review
review-part-2.txt (17.6 KB) - added by Jonathan Lange 13 years ago.
Review part 2

Download all attachments as: .zip

Change History (44)

comment:1 Changed 16 years ago by count0

conch can not handle the new openSSH known_hosts file with hashed host entries.
this feature prevents information leakage.

comment:2 Changed 16 years ago by z3p

Can you attach an example of a hashed host entry (and the host that was hashed)?

comment:3 Changed 16 years ago by count0

this is the content of ~/.ssh/known_hosts after a login to localhost :
|1|FyMY8PDWmm2VbRHWzVcHdXZchuE=|mdlBsVtBFMNwd7Wa+tl4DmeUskg= ssh-rsa
AAAAB3NzaC1yc2EAAAABIwAAAIEAy4h+4L3WoMLc8Ipp1MaQKRW0VeURjsArQkDi8Qf06+Suxu0a6tvepH0dp3CIb9pT4XRiO9F7R1n3euwCAfYYSvZSvWtpGRBWxrO5I6XbbPyV/d/iHYnRWAafO+p5FpBImABrU1IVQipqULYe4/8mG4Y8GddixOHCB5MglSz6U5E=

comment:4 Changed 16 years ago by count0

in hostfile.c (function hash_host) from the openSSH package it seem's like it's
using the HMAC algorithm and a random salt to hash the hostname.

comment:5 Changed 16 years ago by count0

in hostfile.h :
#define HASH_MAGIC	"|1|"
#define HASH_DELIM	'|'

and the function :
char *
host_hash(const char *host, const char *name_from_hostfile, u_int src_len)
{
	const EVP_MD *md = EVP_sha1();
	HMAC_CTX mac_ctx;
	char salt[256], result[256], uu_salt[512], uu_result[512];
	static char encoded[1024];
	u_int i, len;

	len = EVP_MD_size(md);

	if (name_from_hostfile == NULL) {
		/* Create new salt */
		for (i = 0; i < len; i++)
			salt[i] = arc4random();
	} else {
		/* Extract salt from known host entry */
		if (extract_salt(name_from_hostfile, src_len, salt,
		    sizeof(salt)) == -1)
			return (NULL);
	}

	HMAC_Init(&mac_ctx, salt, len, md);
	HMAC_Update(&mac_ctx, host, strlen(host));
	HMAC_Final(&mac_ctx, result, NULL);
	HMAC_cleanup(&mac_ctx);

	if (__b64_ntop(salt, len, uu_salt, sizeof(uu_salt)) == -1 ||
	    __b64_ntop(result, len, uu_result, sizeof(uu_result)) == -1)
		fatal("host_hash: __b64_ntop failed");

	snprintf(encoded, sizeof(encoded), "%s%s%c%s", HASH_MAGIC, uu_salt,
	    HASH_DELIM, uu_result);

	return (encoded);
}

comment:6 Changed 15 years ago by count0

okay, i found out that paramiko (another ssh implementaion for python) has implemented that feature here's the function :


    def hash_host(hostname, salt=None):
        """
        Return a "hashed" form of the hostname, as used by openssh when storing
        hashed hostnames in the known_hosts file.
        
        @param hostname: the hostname to hash
        @type hostname: str
        @param salt: optional salt to use when hashing (must be 20 bytes long)
        @type salt: str
        @return: the hashed hostname
        @rtype: str
        """
        if salt is None:
            salt = randpool.get_bytes(SHA.digest_size)
        else:
            if salt.startswith('|1|'):
                salt = salt.split('|')[2]
            salt = base64.decodestring(salt)
        assert len(salt) == SHA.digest_size
        hmac = HMAC.HMAC(salt, hostname, SHA).digest()
        hostkey = '|1|%s|%s' % (base64.encodestring(salt), base64.encodestring(hmac))
        return hostkey.replace('\n', '')
    hash_host = staticmethod(hash_host)


i'll try to integrate this into twisted conch ...

comment:7 Changed 15 years ago by count0

comment:8 Changed 15 years ago by count0

Owner: set to count0
Status: newassigned

comment:9 Changed 13 years ago by Glyph

#3389 was a duplicate of this.

comment:10 Changed 13 years ago by Glyph

Cc: ericf added
Owner: changed from count0 to Glyph
Status: assignednew

Adding ericf because I believe he's interested in this. Also, I've basically implemented a fix, so I'll make a branch momentarily.

comment:11 Changed 13 years ago by Glyph

Status: newassigned

comment:12 Changed 13 years ago by Glyph

author: glyph
Branch: branches/hashed-1376

(In [24992]) Branching to 'hashed-1376'

comment:13 Changed 13 years ago by Glyph

So that I remember to annotate them, I've discovered some other bugs in known_hosts handling that this branch will also address:

(I've also discovered some other bugs that this branch won't address.)

comment:14 Changed 13 years ago by Glyph

Some other tickets that this should fix:

comment:15 Changed 13 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

Ready for review.

comment:16 Changed 13 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:17 Changed 13 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Status: assignednew
  1. @since tags are missing
  2. the year in the copyright header should be updated

comment:18 Changed 13 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Glyph

Here are a few more review comments. :)

  1. twisted/conch/client/default.py
    1. update copyright date
    2. add module docstring
  2. twisted/conch/test/test_knownhosts.py
    1. The module name in the module docstring should use L{}
    2. lots of whitespace in the import area at the top of the file. We don't usually leave blank lines there.
    3. EntryTestsMixin should document the entry attribute of itself, or that attribute should be replaced with a getEntry method.
  3. twisted/conch/test/test_keys.py
    1. add module docstring
    2. test_fingerprint uses L{Key.fingerprint()}, should be L{Key.fingerprint}
  4. twisted/conch/ssh/keys.py
    1. in the fingerprint method, a list comprehension probably makes more sense than a map/lambda
    2. the fingerprint method should document at least its return type. Probably it should also describe the actual format instead of indirectly documented it by telling people to go learn what OpenSSH's format is.
    3. fingerprint method should have a @since tag
    4. can you remove the stray "#" line at the top of the module?
  5. twisted/conch/interfaces.py
    1. there is trailing whitespace in this file, can you clean it up?
    2. add a module docstring
    3. put a @since tag on IKnownHostEntry
    4. IKnownHostEntry.toString says including where it should say inclusion
  6. twisted/conch/error.py
    1. the docstring of UserRejectedKey may be overly precise. There isn't necessarily any interaction, nor is there necessarily a user. This is a pretty nit picky point, though.
    2. Lots of whitespace in the class docstring for HostKeyChanged
  7. twisted/conch/knownhosts.py
    1. PlainEntry.fromString raises a ValueError if the key doesn't consist of three or more whitespace-delimited parts. Likewise for HashedEntry.fromString. This probably at least merits documentation, maybe tests too as this codepath isn't exercised by any of the existing tests. Actually, I'm wrong about that (according to the coverage stats). I'm not sure where it's covered, but apparently it is. However, the ValueError potentially raised by the next line isn't covered (though I thought for sure it was).
    2. Apparently neither possible ValueError which can be raised by HashedEntry.fromString is covered, though since this is another implementation of the same interface and it's the caller's job to deal with such exceptions, coverage for this may not be necessary. On the other hand, it is important for misformatted lines to cause this exception to be raised, and that's solely up to this code, so maybe it should be tested.
    3. ConsoleUI.warn should probably close the file explicitly. Hey I know, FilePath.setContent (ha ha).
    4. Likewise for ConsoleUI.prompt.
    5. ConsoleUI.opener is documented as a one argument callable, but it's a no-argument callable.
  8. twisted/conch/client/default.py
    1. verifyHostKey docstring refers to _verifyHostKey which I don't see anywhere.
    2. host is documented twice in the docstring of verifyHostKey

This is clearly mostly minor stuff. I like the new code. :) I was a bit surprised that KnownHostsFile doesn't implement any explicitly defined interface. I guess that's because only its verifyHostKey would be on that interface and that method might just be passed in to whatever part of conch is eventually going to interact with this object, rather than the entire instance being passed in.

comment:19 Changed 13 years ago by Glyph

(In [25281]) Update copyright date and add module docstring. re #1376

comment:20 Changed 13 years ago by Glyph

(In [25282]) Address point 2. re #1376

comment:21 Changed 13 years ago by Glyph

(In [25283]) Address point 3. re #1376

comment:22 Changed 13 years ago by Glyph

(In [25284]) Address point 4. re #1376

comment:23 Changed 13 years ago by Glyph

(In [25285]) Address point 5. re #1376

comment:24 Changed 13 years ago by Glyph

(In [25287]) Address thijs's point. re #1376

comment:25 Changed 13 years ago by Glyph

Point 6 struck me as a bit weird:

6.1: the docstring of UserRejectedKey may be overly precise. There isn't necessarily any interaction, nor is there necessarily a user. This is a pretty nit picky point, though.

As you say - pretty nit picky. I'm not sure what else you'd suggest. The "UI" object, for example, describes a user interface; there could be a non-interactive version that doesn't actually interface with a user, but it still makes sense (to me, anyway) to model it that way.

6.2: Lots of whitespace in the class docstring for HostKeyChanged

It looks like the exact same whitespace idiom I'm using everywhere else, to me.

comment:26 Changed 13 years ago by Glyph

(In [25288]) Address point 7.3. re #1376

comment:27 Changed 13 years ago by Glyph

(In [25289]) Address point 7.5. re #1376

comment:28 Changed 13 years ago by Glyph

(In [25291]) Address point 7.4. re #1376

comment:29 Changed 13 years ago by Glyph

(In [25292]) Work towards addressing point 7.3. This adds documentation of the different types of exception which may be raised and traps them specifically, as well as refactoring the common functionality up into a superclass, but does not add test coverage for InvalidEntry. re #1376

comment:30 Changed 13 years ago by Glyph

Oops last point should have been 7.1.

comment:31 Changed 13 years ago by Glyph

(In [25293]) Improve verifyHostKey's docstring to address point 8. re #1376

comment:32 Changed 13 years ago by Glyph

For those of you following along in the book, the only remaining thing to be addressed now is testing for the errors described in points 7.1 and 7.2.

comment:33 Changed 13 years ago by Glyph

Status: newassigned

comment:34 Changed 13 years ago by Glyph

(In [25298]) Tests to address the remainder of points 7.1 and 7.2. re #1376

comment:35 Changed 13 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

I was a bit surprised that KnownHostsFile doesn't implement any explicitly defined interface. I guess that's because only its verifyHostKey would be on that interface and that method might just be passed in to whatever part of conch is eventually going to interact with this object, rather than the entire instance being passed in.

The other reason is that I didn't want to prematurely generalize; I would eventually like to develop alternate implementations of the implied interface here, but I haven't done that yet, and I can formally specify it when I have my second implementation of it. (Yay Python.)

Anyway I'm fairly confident that the response to your feedback will register as a critical hit; let me know what you think.

comment:36 Changed 13 years ago by Glyph

I think this URL may be useful, but I used a bunch of I think undocumented stuff to generate it so I'm not sure...

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/hashed-1376

Changed 13 years ago by Jonathan Lange

Attachment: review-part-1.txt added

First part of review

Changed 13 years ago by Jonathan Lange

Attachment: review-part-2.txt added

Review part 2

comment:37 Changed 13 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Glyph

I've attached my review notes so that you can get going. In summary:

  • The tests make a bunch of assumptions about pregenerated data. This is bad, perhaps necessary, and should probably be addressed in another ticket if you don't want to address it here.
  • The tests assume that the Deferred's fire synchronously. This is going to cause pain later. If the synchronous firing is not part of the API, the tests should change.
  • Lots of whitespace comments.
  • There are a few variables that aren't properly camel-cased (e.g. keytype, fakefile, raiseit).
  • A few inaccuracies in docstrings.
  • And one or two recommendations for cleanup / factoring.

Thanks so much for doing this work. As I mention, it's great to see Conch growing unit tests and well-documented behaviour.

comment:38 Changed 13 years ago by Glyph

(In [25364]) Deal with (final?) review comments. re #1376

comment:39 Changed 13 years ago by Glyph

Keywords: review added
Owner: changed from Glyph to Jonathan Lange

Part 1

Well done! Adding so many actual *unit* tests to Conch is really helpful and gives me confidence that it will become easier to hack on and improve. Thanks.

Thanks! I sure wish it were easier to do, and that the tests covered more existing functionality, but I'll take what I can get. Overall I'm very happy with the testability and the structure of this new interface and I'm looking forward to pushing it through the rest of conch.

The big thing is that the code consistently assumes that the Deferreds are going to be fired synchronously. This is only really acceptable if that's what the public interface declares. Otherwise, the tests should be changed to return Deferreds and add their assertions in callbacks.

I really prefer to write tests for Deferred-generating APIs this way - asserting that the Deferred hasn't fired, then asserting that it has. You get faster failures that way - no waiting for the timeout in the case where you forgot to fire it, just an instant failure. There's also the added benefit of not getting the whole mess of reactor junk involved when it really has nothing to do with the system under test.

In this particular case, the object whose responsibility it is to decide when the Deferred fires is the UI. I thought that would be clearer from the 'ui.promptDeferred.callback()' calls that were strategically placed in most of the tests that had this property. It is an important property of the interface - *the* important property of the interface, really - that when the UI fires its Deferred, the result of the verifyHostKey Deferred is immediately fired. If someone wants to change that to rely on some other Deferreds from some other unspecified sources, they really should be changing these tests as well.

One step up the chain, when you're dealing with a Deferred that someone else is supposed to fire (arguably from a system which has been tested to fire its Deferred correctly and not let the timeout take over, and wants to do some I/O or timed events), it's a bit more reliable to return the Deferred and write tests in callbacks, since it's not the SUT's fault if the Deferred doesn't fire (i.e. the test which should fire cleanly and quickly is elsewhere). This is most helpful when you're a couple of levels of abstraction removed from what, exactly, is supposed to fire the Deferred, and you don't want to pay attention to that in your tests. I am finding that, as I become a better TDDer, I write less and less code like this: the underlying system should pretty much _always_ expose which events it's waiting for. It's only when you get to the last link in the chain, the integration test that makes sure the events "really" fire, that you start returning Deferreds and spinning the reactor.

I don't want to downplay that; IMHO trial is a very good environment for writing integration tests and I think the seamless transition between unit-test writing and integration-test writing is very pleasant. So, don't take this to mean "I think returning Deferreds from tests is a bad idea". It's just that when you're working on code that deals with Deferreds in a test-driven way, especially the code that is returning, rather than just adding callbacks to, Deferreds, you want to be very precise about when and how they fire, and that means asserting things about them synchronously as you go rather than returning them.

If you want to discuss this in some more depth it would probably be good mailing-list fodder.

The other high level thing is that the tests use pre-made data and make assumptions about its exact nature. This is sometimes simply what you have to do, but it would be nice to think about how we can generate the data as we need it, specifying the interesting properties in the tests. Perhaps that's for another ticket.

If I had to do this branch over again,

AIUI, there should be three lines of VWS between each of these variables.

The coding standard says "top level classes and functions". It doesn't actually say anything about assignments / variable definitions. The Divmod coding standard specifically says "module-level suites", and these aren't suites, they're expressions.

I realize there's some ambiguity there, and maybe we should tighten it up. I'm inclined to leave it as-is, though; the main motivation for the fixed-number-of-blank-lines rule is to allow for a distinction between discretionary whitespace (individual blank lines) and moving-on-to-the-next-thing whitespace (next class, next method, next function). The definitions-of-constants section at the top of a module strikes me as like the inside of a method body, where one-or-no blank lines should separate constants.

If there is some agreement perhaps we should add something to that effect to the coding standard.

If I read these tests correctly, the entry attribute actually needs to be more than just an IKnownHostEntry. It also has to conform to certain expectations of its content. Please document this.

I already did, in "@ivar entry". In the test that requires more than the interface provides, I've documented a little bit more.

This docstring refers to "PlainEntry", but aren't these tests for any IKnownHostEntry provider?

Fixed.

Man, all this sample data. Do you have any ideas about how we could generate the data, rather than using pre-existing stuff that mysteriously depends on things like "198.49.126.131"?

If I had any really good ideas I probably would have implemented them already :). A mediocre one though: I think we should file a ticket to merge this branch's sample data with twisted.conch.test.keydata.

Why aren't there any host IP tests here?

Hashed entries don't support that syntax feature. I've added a comment to that effect, and explaining why, to the class docstring.

That's a cute performance hack, but it's a little hard to read. I'm not insisting that you change it though: follow your heart.

Hmm. It was actually for readability, not performance. And to make Emacs indent the parameter list at a different depth than the docstring and implementation. Coming back to this a few days later, _I_ still find the argument list easier to read that way, so I think I'll leave it as is ;).

The docstring says "two entries", so it's a little confusing to assert that there are six entries. Can you please change the docstring to warn readers of this?

That was just stale, from when there were only two types rather than three, and only one way to parse them rather than several ways to detect unparseable entries. Updated.

You might want to add a custom assertion method for those last few. Your call.

On the grounds that it would require jumping around more to read the tests, I'll leave it as is. Agreed that it's really close to the line, though.

I generally lean toward self.assertEqual([], list(kh._entries)), since that gives me a better error message if it screws up. No big deal though.

Good point, done.

Why do you keep reconstructing FilePath(pn)?

Easier to read than repeated calls to restat().

Maybe it should be in its own test then?

The comment as written wasn't too descriptive. I've updated it to explain.

So, this test and test_savingAddsEntry would both pass if addHostKey wrote to disk. Do you think it's worth adding an assertion that confirms that save() is required before data on disk changes?

I can't figure out how to screw up the code such that these tests will pass in a misleading way, so I don't think so. I might be misunderstanding your comment though.

Interesting. What happens if you add a key for the same host twice? (Both at the API level and to the salt.)

You can't actually drive the UI to make this code do that (I think) so it's not a terribly interesting case. As it happens the salt will be different, but I didn't have to write any code to make it so, and it's not an important property (both because you can't get the UI to do it, and because later only the first entry will be looked at) so I didn't bother with a test.

So the interface is that it fires a synchronous Deferred? If not,

return d.addCallback(self.assertEqual, True)

would be simpler code and would make the test more general.

No. The interface is that it fires its Deferred when the UI has replied to it - which, in this case, is immediately.

See above for the more general comments.

This took me a while to understand. The reason is that it's using the list to avoid returning Deferreds from tests. I don't really like this style and would prefer it if you changed the code to just return Deferreds. However, I think it would also be OK if you just extended the docstring of this method to say more about the Deferred-dodging and why you are doing it.

I've added a brief explanation. I'm not entirely happy with it, because I think the reasoning for the Deferred-dodging should be obvious. However, if it confused you, it's likely to confuse someone else, so I've added a brief explanation.

It'd be nice if 'kh' got a more meaningful name.

Changed everywhere.

Should probably be fakeFile. Your call.

Changed everywhere.

os.environHOME? = tmpdir

Extracting this to a separate method would make the test clearer. Your call.

Definitely a good idea; done.

Part 2

Why not use base64.encodestring?

I started off using base64.b64encode which did exactly what I wanted, but that's not available in python2.3. 'encodestring' adds a "\n" anyway, so I'd need to make a function to strip it off anyway; and I don't see what module the 'base64' module adds over 'binascii', except for more sloooow python method calls, indirection, and different method names.

The contents of key-type is ambiguous, since Conch has more than one way of referring to type.

Yeah, that's kind of lame. Some thoughts off the top of my head:

  • twisted.python should have an enumerated type in it so we can make having stuff like this easy
  • conch should use strings less in general
  • specifically somebody (you? :)) should file a ticket for adding an object for determining an SSH key's type which can be converted to the various ways in which external systems refer to key types.

I think the way to talk about this key-type in Conch is to say "ssh key type" (see Key.sshType for example.)

Fixed.

I was just thinking, shouldn't this be a classmethod. Too much Python 2.4. :\

FWIW, Divmod has started using decorator syntax, so when we drop 2.3...

Oh, I've seen this in tests. You should probably use HashedEntry.MAGIC rather than '|1|' when you want to include this in strings for testing -- most of the time anyway.

OK, replaced everywhere it appeared as a literal by itself, at least.

Spaces around +.

Clearly I need some kind of behavioral conditioning therapy.

ConsoleUI

This is cool. I guess someday it should find a home elsewhere.

It should also grow some additional methods. The ideal shape of this API would allow the code to ask much more specific questions of the UI with more structured data, like, "It looks like the hostname X is now mapped to the same key you have another host mapped to. Would you like me to delete this automatically?" or somesuch.

I mentioned the "Mystery Guest" thing in a previous review. However that works itself out applies here.

Oooh. "Mystery Guest". Good pattern to know about. I think we should punt on it for now, working it out for real would just be too hard. But it's something I'll be thinking about.

Is all that VWS part of the standard? It's quite disconcerting seeing the type split from the variable description like that.

There is no standard on this just yet, and as such, I'm going to leave it how it is for now because it makes life easier for me when line-wrapping with emacs :).

Maybe you can make a standard to overrule me on this, and then I'll go grumble and write some elisp.

comment:40 in reply to:  39 Changed 13 years ago by Jonathan Lange

Keywords: review removed
Owner: changed from Jonathan Lange to Glyph

Replying to glyph:

The big thing is that the code consistently assumes that the Deferreds are going to be fired synchronously. This is only really acceptable if that's what the public interface declares. Otherwise, the tests should be changed to return Deferreds and add their assertions in callbacks.

I really prefer to write tests for Deferred-generating APIs this way - asserting that the Deferred hasn't fired, then asserting that it has. You get faster failures that way - no waiting for the timeout in the case where you forgot to fire it, just an instant failure. There's also the added benefit of not getting the whole mess of reactor junk involved when it really has nothing to do with the system under test.

These are arguably bugs in Trial itself.

In this particular case, the object whose responsibility it is to decide when the Deferred fires is the UI. I thought that would be clearer from the 'ui.promptDeferred.callback()' calls that were strategically placed in most of the tests that had this property. It is an important property of the interface - *the* important property of the interface, really - that when the UI fires its Deferred, the result of the verifyHostKey Deferred is immediately fired. If someone wants to change that to rely on some other Deferreds from some other unspecified sources, they really should be changing these tests as well.

This is a much more convincing point :)

...

If you want to discuss this in some more depth it would probably be good mailing-list fodder.

Definitely. I don't quite have the energy to raise it though. :)

The other high level thing is that the tests use pre-made data and make assumptions about its exact nature. This is sometimes simply what you have to do, but it would be nice to think about how we can generate the data as we need it, specifying the interesting properties in the tests. Perhaps that's for another ticket.

If I had to do this branch over again,

*nod* Fair enough.

AIUI, there should be three lines of VWS between each of these variables.

The coding standard says "top level classes and functions". It doesn't actually say anything about assignments / variable definitions. The Divmod coding standard specifically says "module-level suites", and these aren't suites, they're expressions.

I realize there's some ambiguity there, and maybe we should tighten it up. I'm inclined to leave it as-is, though;

Fine by me.

the main motivation for the fixed-number-of-blank-lines rule is to allow for a distinction between discretionary whitespace (individual blank lines) and moving-on-to-the-next-thing whitespace (next class, next method, next function).

The motivation should probably be mentioned in the standard, fwiw.

If there is some agreement perhaps we should add something to that effect to the coding standard.

Fine by me.

So, this test and test_savingAddsEntry would both pass if addHostKey wrote to disk. Do you think it's worth adding an assertion that confirms that save() is required before data on disk changes?

I can't figure out how to screw up the code such that these tests will pass in a misleading way, so I don't think so. I might be misunderstanding your comment though.

Well, if addHostKey called save(), then the tests would pass. The interface doesn't say that you MUST call save, so it's not strictly an error.

If you wanted to add such a test, you'd just change the "save" test to check that the file contents are unchanged just before the save() call.

Interesting. What happens if you add a key for the same host twice? (Both at the API level and to the salt.)

You can't actually drive the UI to make this code do that (I think) so it's not a terribly interesting case. As it happens the salt will be different, but I didn't have to write any code to make it so, and it's not an important property (both because you can't get the UI to do it, and because later only the first entry will be looked at) so I didn't bother with a test.

Leaving the behaviour undefined is ok for now.

This took me a while to understand. The reason is that it's using the list to avoid returning Deferreds from tests. I don't really like this style and would prefer it if you changed the code to just return Deferreds. However, I think it would also be OK if you just extended the docstring of this method to say more about the Deferred-dodging and why you are doing it.

I've added a brief explanation. I'm not entirely happy with it, because I think the reasoning for the Deferred-dodging should be obvious. However, if it confused you, it's likely to confuse someone else, so I've added a brief explanation.

Thanks.

Part 2

Why not use base64.encodestring?

I started off using base64.b64encode which did exactly what I wanted, but that's not available in python2.3. 'encodestring' adds a "\n" anyway, so I'd need to make a function to strip it off anyway; and I don't see what module the 'base64' module adds over 'binascii', except for more sloooow python method calls, indirection, and different method names.

Fair enough.

I was just thinking, shouldn't this be a classmethod. Too much Python 2.4. :\

FWIW, Divmod has started using decorator syntax, so when we drop 2.3...

\o/

I mentioned the "Mystery Guest" thing in a previous review. However that works itself out applies here.

Oooh. "Mystery Guest". Good pattern to know about. I think we should punt on it for now, working it out for real would just be too hard. But it's something I'll be thinking about.

+1.

The term is from Mezsaros' "xUnit testing patterns" book, the first third of which is required reading.

Is all that VWS part of the standard? It's quite disconcerting seeing the type split from the variable description like that.

There is no standard on this just yet, and as such, I'm going to leave it how it is for now because it makes life easier for me when line-wrapping with emacs :).

Maybe you can make a standard to overrule me on this, and then I'll go grumble and write some elisp.

I don't care that much, tbh.

Please land. :)

jml

comment:41 Changed 13 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [25365]) Fixes for the conch client's known_hosts parsing and host key verification.

Fixes #1376 Fixes #1301 Fixes #3494 Fixes #3496 Fixes #1292 Fixes #3499

The main thrust of this change is that it adds a new module to conch, twisted.conch.client.knownhosts, which provides a structured representation of OpenSSH's known_hosts file. This is a big step in the direction of modularizing conch's support for verifying host keys and storing the results of that verification, although the internal connection APIs are mostly unchanged at this point, they could now easily be adjusted to speak to an abstract interface and still manipulate the user's actual known_hosts entries rather than completely overriding them.

The individual bugs which this change fixes are too numerous to bother describing twice; have a look at the tickets themselves more information. Suffice it to say that the conch client is now a lot more reliable (although it still has a long way to go).

In addition to making the conch client more stable, the new API allows for some programmatic manipulation of known_hosts files, which might be independently useful.

Additionally, although the review notes some ways in which the tests can still be improved, the tests for this module can be cited as a much better example for conch contributors as to how unit tests within conch should look. While these tests are covering almost exclusively new code, they are covering functionality which has existed (and been broken) for quite some time.

Author: glyph

Reviewers: exarkun, jml

comment:42 Changed 11 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.