search

clausecker / freefare

Go bindings for the libfreefare
GNU Lesser General Public License v3.0
18 stars 3 forks source link

Ultralight PR #8

Closed graynk closed 4 years ago

graynk commented 4 years ago

Bugfixes are pretty small and can be merged easily. Key derivation is a bit harder: I don't know the thought process of maintainers of libfreefare/libnfc, but there hasn't been a release for 5 years now, despite them constantly adding new features and breaking API. Current version works for current state of master branch, but that may very well change, I'm not sure what's the best course of action here.

clausecker commented 4 years ago

Thanks for the pull request. Unfortunately, it edits the 0.3 branch with incompatible changes (i.e. it won't compile with libfreefare 0.4.0 anymore). Can you edit the pull request so it only affects the dev folder instead? This way, compatibility is not infringed upon and I can eventually cut a 1.0 release.

If you don't want to invest the time doing that it's fine. I can do these changes myself, too.

graynk commented 4 years ago

Sure thing, I should've done that from the beginning

clausecker commented 4 years ago

Thanks a lot! I would welcome another PR for the documentation, but if you don't feel like doing that, I can add it myself. I have some trouble testing your code at the moment as I can't get the current development version of libfreefare to compile, but I'll see what I can do.

graynk commented 4 years ago

Regarding compiling libfreefare: I'm on Ubuntu 20.04 and I was able to compile it today using instructions from README.MD:

sudo apt-get install autoconf automake git libtool libssl-dev pkg-config
autoreconf -vis
./configure --prefix=/usr
make
sudo make install
clausecker commented 4 years ago

Yeah, the main problem is that I believe I need a development version of the libnfc. They too haven't cut a release in years. It's infuriating.

graynk commented 4 years ago

Oh yeah, I had to compile both libnfc and libfreefare from master, they're pretty tightly coupled together.

clausecker commented 4 years ago

I see. I suppose a new release of this project must wait until both have cut new releases then. I hope they won't take too long for that.

I already dread updating my libnfc wrapper, too. Haven't worked with NFC tags in years.

clausecker commented 4 years ago

I'm currently writing the relevant documentation. Why have you chosen to tie the tag used in (*MifareKeyDeriver).UpdateUID to the same tag used when it was created? Would you mind if I changed this to support arbitrary uids?

graynk commented 4 years ago

I hadn't tested if MifareKeyDeriver can be reused just by calling Begin again and setting UID from another tag, but if it can, then it seems like a better way to do things, sure. There's no real need to save the tag itself either, NewAn10922 can be it's own function in key-deriver.go that takes just the keyType as input. The problem is that we need TranslateError in MifareKeyDeriver functions that only Tag has now

clausecker commented 4 years ago

Sure. The UpdateUID function seems to just write some data derived from the UID into the buffer, so it is plausible that it can be used with any UID. I have retouched your code slightly, will push the changes shortly. Kindly let me know if you consider them to be acceptable.

clausecker commented 4 years ago

I've touched up your code. Please let me know if this is acceptable for your desired usage. Also let me know your real name so I can add your authorship to the copyright headers if desired.

graynk commented 4 years ago

I've noticed a small typo and left a comment there. Otherwise LGTM, but I won't be able to actually test this with real hardware until the quarantine is over, so it'll be a month or two before I can get to my reader and tags.

Sure, my real name is Nikita Karpukhin :)

clausecker commented 4 years ago

Thanks a lot. Your credit has been noted and your objection fixed.

clausecker commented 3 months ago

Hi Nikita,

Do you recall which particular version of freefare you tested this one with?

clausecker commented 3 months ago

Hi Nikita,

I'll back out your changes and retract the 0.4.0 release as I have been unable to find any version of libfreefare that builds with this patch set. Before the diversification features were committed, upstream broke compatibility by renaming a bunch of symbols, breaking the build. There does not seem to be any commit that has both the diversification features and the old identifiers prior to being renamed.

Once libfreefare cuts a new release I'll look into updating this library to the new symbol names and then cut a 1.0.0 release with your diversification patch set hopefully.

graynk commented 3 months ago

Hey @clausecker. I don't remember much about this, since I haven't been working with freefare for a long long while, but I vaguely remember linking against whatever was in master at the time, and judging by the timeframe - it must've been this?

https://github.com/nfc-tools/libfreefare/commit/0480b7248e566e7604792c7ec992a2e166ce33ba

But again - no real memory of that and no real way to test for it either. Feel free to revert the changes, of course

clausecker commented 3 months ago

Hi @graynk, I managed to find a version this builds with in the meanwhile. Thanks for checking anyway!