Conversation
| if(idx >= libssh2_agent_get_backend_list_size()) | ||
| return NULL; | ||
|
|
||
| return agent_supported_backends[idx].name; |
There was a problem hiding this comment.
How about using purely names instead of direct indexes in the API?
It would avoid exposing the internals and also avoid a bunch of OOB
issues. It'd also not need a listing function by documenting the names,
and returning an error if the selected name is not supported.
There was a problem hiding this comment.
Using purely names is flaky compared to an opaque index. If a new agent backend is added, the library using libssh2, in my case libgit2, would need to be updated. To have a generic solution, there must be a way to know how many agent backends exist, and a way to use one particular one.
In the end, I chose to use an index because it requires the fewest changes to libssh2. But I'm more than happy to add more functionality to avoid the index.
Without an index, I can't see any other way than to have 2 additional functions to enumerate the agent backend using a first- and next-scheme, which would return an opaque agent backend handle. Then all other functions would take the agent backend handle as input, not an index anymore.
Would that be a better solution? If so, I can implement it.
There was a problem hiding this comment.
So, it could be something like:
- struct _LIBSSH2_AGENT_BACKEND { ... }
- typedef struct _LIBSSH2_AGENT_BACKEND LIBSSH2_AGENT_BACKEND;
- LIBSSH2_AGENT_BACKEND* libssh2_agent_get_first_backend()
- LIBSSH2_AGENT_BACKEND* libssh2_agent_get_next_backend(LIBSSH2_AGENT_BACKEND *prev)
- void libssh2_agent_set_backend_to_use(LIBSSH2_AGENT *agent, LIBSSH2_AGENT_BACKEND *backend);
- const char *libssh2_agent_get_backend_name(LIBSSH2_AGENT_BACKEND *backend);
- etc.
It's a cleaner interface, no question.
There was a problem hiding this comment.
Why do you need to have iterator over the list of supported
backends?
If you want to force-set a specific one, e.g. Pageant, you pass
libssh2_agent_prefer("Pageant") which returns success
if it exists and fail if missing. On fail, you may pass the next
preferred one, or be okay with the default. Is there a further
goal?
If a user absolutely must know the full list of supported backends,
they could perhaps be returned as a space-separated static list,
e.g. as part of the recently added libssh2_build_options() return
value. Or there may be an API to ask libssh2 if a backend with
a given name exists, without trying to set it. Though this list is
fairly static and doesn't seem to be a must to select a particular
one, esp. on Windows.
There was a problem hiding this comment.
The only reason I proposed the iterator is that you didn't want the index. The index is just an integer; it doesn't reveal anything about the library's internals, except that something is presented as an array. I prefer using actual APIs/functions rather than parsing strings. Using an enumeration for the agent backends could be another simpler alternative to using an index.
There was a problem hiding this comment.
The current API proposal is also in iterator, only a simplified one.
I'm strongly not a fan of an interface exposing indexes to internal
tables. Esp. for things that already have unique identifiers.
My understanding is that you want to override the backend
to a preferred one.
5 new APIs to maintain and 300 lines to do this seems excessive
to me.
Would libssh2_agent_set_backend("Pageant") work?
There was a problem hiding this comment.
Yes, I can get my changes in libgit2 to work with it. I'll update the PR accordingly.
There was a problem hiding this comment.
But there is one pretty big gotcha. If libgit2 doesn't accept the changes needed to support the new libssh2 API, because for libgit2 it means importing libssh2 internal details, I'm back to a scenario with one fork for libssh2 and one fork for libgit2.
In Windows, the SSH agent is a mess. There is a pretty nice tool called omniSSHAgent to solve some of those issues. So I was seriously considering contributing more agent backends to libssh2, in the spirit of omniSSHAgent. If I did that, it would mean libgit2 would need to be modified every single time a new agent backend is added to libssh2. I know from having discussed with the libgit2 guys that they will refuse this.
Internally: make the agent backend guards unique for each backend, make them contain the backend name. Use them in `agent.c`, move them to a separate header and also reuse them from `version.c`. Ref: #2285 (comment) Follow-up to 2673970 #2178 Closes #2312
I have issues using libgit2 with libssh2 because both have their own idea of “try another agent.” The problem arises under Windows because there are several backends. libssh2 will pick the first agent backend which can be connected, without any notion that the keys needed by libgit2 are in a different agent.
To fix this, there are 2 options:
Since this issue is currently purely under Windows, I chose the second simpler option. And this is the core of this PR.