First of all. Awesome. Thanks for jumping in on this.

(In reply to comment #0)
> However, I'm currently blocked on finding the code that determines which
> PKCS#11 module (gnome2 or ssh) responds to the request from high-level access.
> I believe the function is related to the gkm_module_register_factory () call,
> but I am unsure.

This happens in the wrap-layer. It accepts a bunch of PKCS#11 modules in gkm_wrap_layer_add_module() and then wraps them into one big module with each lower level modules slots showing through in its wrapped module.

Created attachment 251046 [details] [review]
WIP patch

I spoke with Stef at guadec, and perhaps extending the PKCS#11 ssh-store module would have worked, but it would have proved difficult to get all the information that we need to serialize and write the key data (namely, the encryption passcode used for the key). So, a easier way we came up with was to implement a new GcrImporter that deals only with SSH keys (the PKCS#11 ssh-store module route meant that we would have used the PKCS11 GcrImporter), which circumvents much of the unneeded complexity of using PKCS11 when 'importing a SSH key' essentially just requires writing it to ~/.ssh/

I have a WIP patch that implements this GcrOpensshImporter class, and hooks it up into the rest of gcr. 

Current limitations:

- only the synchonous import is implemented (I just need to learn the GTask API)
- importing doesn't preserve the filename (It just sticks it in ~/.ssh/imported_id_rsa -- I believe I can modify GcrParsed to store the filename)
- only unencrypted rsa for now (I need to modify GcrParsed to store the entire contents of the key, and add a way for a GcrImporter to get this data out)

After these issues are resolved, we can add the capability to automatically generate the public key from the imported private key via ssh-keygen -y. This means that we should not support importing ssh public keys (unless it's useful to just concatenate the public key to ~/.ssh/authorized_keys ?) because when importing the private key we can put both it and the corresponding public key in ~/.ssh

Just a clarification: since only synchronous import is implemented (as of now -- I'm working on async today), this patch doesn't enable importing from seahorse since seahorse only calls the async function.

gnome-keyring-3 import <private-rsa-key> will demonstrate the functionality though since it uses the synchronous function by default.

Review of attachment 251046 [details] [review]:

Nice. Thanks for working on this. I hope you don't mind that I've done a bit of review here.

A few more things to note. I realize we already talked about these:

 1. Should get the outer PEM data from GcrParsed directly. We talked about
    adding gcr_parsed_get_data_for_format().
 2. Implement gcr_parsed_get_filename() so that the SSH importer can tell
    if it's an openssh style filename (which the only indication that it is
    an ssh key).
 3. Add some way to prioritize a the GcrImporter, so that the SSH importer
    can be first if it sees an SSH filename

1) and 2) above should probably be separate patches.

::: egg/egg-armor.c
@@ +255,3 @@
 	g_return_val_if_fail (*decoded, FALSE);
 
+        /* todo shouldn't use step */

Keep in mind that OpenSSL barfs if the lines are the wrong length. But happy for another solution here...

::: gcr/gcr-openssh-importer.c
@@ +43,3 @@
+	GQueue *queue;
+	GTlsInteraction *interaction;
+	GArray *imported;

Probably won't (or can't) track the key fingerprints of what was imported.

@@ +50,3 @@
+        gsize n_block;
+        GcrDataFormat format;
+};

Why can't you just keep a reference to the GcrParsed? That's how it was designed to be used? I may be missing something though.

@@ +72,3 @@
+
+	while (!g_queue_is_empty (self->pv->queue))
+		gck_attributes_unref (g_queue_pop_head (self->pv->queue));

The queue doesn't have GckAttributes* in this case. This will probably segfault.

@@ +91,3 @@
+calculate_label (GcrOpensshImporter *self)
+{
+        return g_strdup (_("OpenSSH: ~/.ssh/"));

I think we can do better here. What do you think about "SSH Keys: ~/.ssh"

Also if this is just a static string, then we can just drop these calculate_xxx() functions. See below.

@@ +135,3 @@
+	switch (prop_id) {
+	case PROP_LABEL:
+		g_value_take_string (value, calculate_label (self));

Should we just use g_value_set_string() and a static string?

@@ +138,3 @@
+		break;
+	case PROP_ICON:
+		g_value_take_object (value, calculate_icon (self));

Should we just call g_themed_icon_new() directly?

@@ +144,3 @@
+		break;
+	case PROP_URI:
+		g_value_take_string (value, calculate_uri (self));

Should we just use g_value_set_string() and a static function?

@@ +184,3 @@
+        format = gcr_parsed_get_format (parsed);
+	if (format != GCR_FORMAT_DER_PRIVATE_KEY_RSA)
+		return NULL;

I guess this is proof of concept, because it doesn't support other key types.

@@ +203,3 @@
+        gint n_outer;
+
+        ssh_dir = g_strconcat(g_get_home_dir(), G_DIR_SEPARATOR_S, ".ssh", 

You probably want the simpler g_build_filename().

@@ +205,3 @@
+        ssh_dir = g_strconcat(g_get_home_dir(), G_DIR_SEPARATOR_S, ".ssh", 
+                              NULL);
+        if (g_mkdir_with_parents (ssh_dir, 0755) == -1) {

I don't think the permission here is correct. I think 0700 is the right mode, but please verify.

@@ +206,3 @@
+                              NULL);
+        if (g_mkdir_with_parents (ssh_dir, 0755) == -1) {
+            g_critical("couldn't create ~/.ssh/ directory");

I don't think this should be a g_critical. We should propagate this error using G_FILE_ERROR as domain and g_file_error_from_errno(errno) as the code, and an appropriate message.

@@ +214,3 @@
+        outer_data = egg_armor_write(pi->block, pi->n_block, 
+                g_quark_from_static_string ("RSA PRIVATE KEY"), NULL,
+                &n_outer);

I imagine this is where we really want to get the outer data directly (so password sticks around, etc.). But I understand this initial patch is proof of concept.

@@ +218,3 @@
+        file_path = g_strconcat(ssh_dir, G_DIR_SEPARATOR_S, "imported_id_rsa",
+                                NULL);
+        if (!g_file_set_contents (file_path, outer_data, -1, NULL)) {

Need to propagate error and no g_critical().

@@ +223,3 @@
+        }
+        if (g_chmod(file_path, 0600) == -1) {
+            g_critical("couldn't change file permissions");

Ditto.

@@ +231,3 @@
+        g_free(key_contents);
+        g_free(pi->block);
+        g_free(pi);

Lotsa leaks here, given the above returns.

@@ +251,3 @@
+
+	block_unowned = gcr_parsed_get_data (parsed, &n_block); //why is ths needed?
+        block = g_memdup(block_unowned, n_block);

I think we can just ref the GcrParsed and queue that way.

::: gcr/gcr-openssh-importer.h
@@ +61,3 @@
+GcrImporter *           _gcr_openssh_importer_new              (void);
+
+const gchar **          _gcr_openssh_importer_get_imported     (GcrOpensshImporter *self);

This function may not make sense for the openssh importer, unless there's something to call it.

::: gcr/gcr-parser.c
@@ -2768,3 @@
  * Get the raw data block for the parsed item.
  *
- * Returns: (transfer full) (array length=n_data) (allow-none): the raw data of

Could you make a separate patch out of this?

Created attachment 253952 [details] [review]
Annotation fix

Created attachment 253953 [details] [review]
SSH Private Key Import

Created attachment 253954 [details] [review]
filename patch

Created attachment 253955 [details] [review]
importer priority

(In reply to comment #4)
> Review of attachment 251046 [details] [review]:
> 
> I hope you don't mind that I've done a bit of review here.
The more reviews the better! Thanks!

> 
> A few more things to note. I realize we already talked about these:
> 
>  1. Should get the outer PEM data from GcrParsed directly. We talked about
>     adding gcr_parsed_get_data_for_format().
It appears GcrParsed already keeps track of different formats via a linked list. I exposed this, by modifying gcr_parsed_ref, but doesn't appear to be threadsafe for some reason. I can't figure it out, any ideas? Patch 2 deals with this 

>  2. Implement gcr_parsed_get_filename() so that the SSH importer can tell
>     if it's an openssh style filename (which the only indication that it is
>     an ssh key).
Patch 3 deals with this in an okay way, but it's not perfect since ensuring that Parseds have a filename is spread out among all the functions that create Parseds (and there are many of these functions).

>  3. Add some way to prioritize a the GcrImporter, so that the SSH importer
>     can be first if it sees an SSH filename
Patch 4 has some rough ideas, and some thoughts on where changes need to be made. 

> 
> ::: egg/egg-armor.c
> @@ +255,3 @@
>      g_return_val_if_fail (*decoded, FALSE);
> 
> +        /* todo shouldn't use step */
> 
> Keep in mind that OpenSSL barfs if the lines are the wrong length. But happy
> for another solution here...
I misread the code. I thought step wasn't looping and thus ran the risk of failing to read the entire file. The function containing this line is used in a loop though, so all is good.
 
> ::: gcr/gcr-openssh-importer.c
> @@ +43,3 @@
> +    GQueue *queue;
> +    GTlsInteraction *interaction;
> +    GArray *imported;
> 
> Probably won't (or can't) track the key fingerprints of what was imported.
I don't plant to keep track of fingerprints, but I believe I have to keep the GTlsInteraction there since a property of that type is required by GcrImporterIface (though I don't use it for anything).


> @@ +50,3 @@
> +        gsize n_block;
> +        GcrDataFormat format;
> +};
> 
> Why can't you just keep a reference to the GcrParsed? That's how it was
> designed to be used? I may be missing something though.
Good idea. I coded it up, but as I mentioned, there are some bugs I'm having trouble tracking down.


> @@ +91,3 @@
> +calculate_label (GcrOpensshImporter *self)
> +{
> +        return g_strdup (_("OpenSSH: ~/.ssh/"));
> 
> I think we can do better here. What do you think about "SSH Keys: ~/.ssh"
That's better. Should ~ be expanded to an absolute path though?

> Also if this is just a static string, then we can just drop these
> calculate_xxx() functions. See below.
I dropped most of them but kept the uri one. Should we use file:// or ssh:// or something else? I see you use gnupg:// in gnupgp-importer, but I didn't see that here https://en.wikipedia.org/wiki/URI_scheme#Official_IANA-registered_schemes

>
> 
> @@ +205,3 @@
> +        ssh_dir = g_strconcat(g_get_home_dir(), G_DIR_SEPARATOR_S, ".ssh", 
> +                              NULL);
> +        if (g_mkdir_with_parents (ssh_dir, 0755) == -1) {
> 
> I don't think the permission here is correct. I think 0700 is the right mode,
> but please verify.

You're correct. 700 is the proper mode.

> 
> @@ +206,3 @@
> +                              NULL);
> +        if (g_mkdir_with_parents (ssh_dir, 0755) == -1) {
> +            g_critical("couldn't create ~/.ssh/ directory");
> 
> I don't think this should be a g_critical. We should propagate this error using
> G_FILE_ERROR as domain and g_file_error_from_errno(errno) as the code, and an
> appropriate message.
> 
These g_critical errors have been updated to work with GError.

Created attachment 255270 [details] [review]
SSH Private Kye Import -  fixes build issue due to function reordering. 

Previous version didn't compile due to function definition ordering. This fixes the build. Still working on tracking down aforementioned bugs.

Review of attachment 253953 [details] [review]:

Thanks for the work. Did some review below. Hope it's a help. In addition there are some pedantic style issues to help match the surrounding code.

::: gcr/gcr-openssh-importer.c
@@ +85,3 @@
+calculate_uri (GcrOpensshImporter *self)
+{
+	return g_build_filename("file://", g_get_home_dir(), ".ssh", NULL);

The URI is just used as a unique identifier so an app can later import to the same place. Would recommend "x-openssh://" or something like that to indicate: the default location for openssh keys. When this becomes constant, then me would just remove this function all together.

@@ +165,3 @@
+	iface->import_async = _gcr_openssh_importer_import_async;
+	iface->import_sync = _gcr_openssh_importer_import_sync;
+	iface->import_finish = _gcr_openssh_importer_import_finish;

This function needs to follow the definitions of all these functions, or you get all sorts warnings/errors from gcc by referencing functions that have not yet been declared:


gcr-openssh-importer.c:160:23: error: '_gcr_openssh_importer_import_sync' undeclared (first use in this function)
  iface->import_sync = _gcr_openssh_importer_import_sync;
                       ^
gcr-openssh-importer.c:161:25: error: '_gcr_openssh_importer_import_finish' undeclared (first use in this function)
  iface->import_finish = _gcr_openssh_importer_import_finish;

... and so on ...

@@ +169,3 @@
+
+gboolean
+_gcr_openssh_importer_can_import (GcrParsed *parsed, GcrParsed *wrapper) {

Spacing here. brace should be on new line, and each argument should be on new line.

@@ +170,3 @@
+gboolean
+_gcr_openssh_importer_can_import (GcrParsed *parsed, GcrParsed *wrapper) {
+                                  GcrDataFormat format, wrapper_format;

Indentation issue, see other functions.

@@ +192,3 @@
+	wrapper = gcr_parsed_get_wrapper (parsed);
+
+	if (!(_gcr_openssh_importer_can_import (parsed, wrapper)))

Extra paren unnecessary.

@@ +211,3 @@
+	wrapper = gcr_parsed_get_wrapper (parsed);
+
+	if (!(_gcr_openssh_importer_can_import(parsed, wrapper)))

Extra paren again, and missing space.

@@ +214,3 @@
+		return FALSE;
+
+	gcr_parsed_ref(wrapper);

All functions calls throughout here need a space between the name and the paren. Lots of instances of this below.

@@ +237,3 @@
+		g_set_error(error, G_FILE_ERROR, g_file_error_from_errno(errno),
+		            _("SSH key directory does not exist "
+		              "and could not be created."));

Suggest adding directory name to error using:

"and could not be created: %s"), ssh_dir);

@@ +238,3 @@
+		            _("SSH key directory does not exist "
+		              "and could not be created."));
+		return FALSE;

Leaks ssh_dir.

@@ +247,3 @@
+
+		file_path = g_strconcat(ssh_dir, G_DIR_SEPARATOR_S,
+		                        "imported_id_rsa",

How does this deal with multiple keys being imported one after the other?

@@ +251,3 @@
+		if (!g_file_set_contents(file_path,
+		                         (const gchar *)data, n_data, error)) {
+			g_assert (error == NULL || *error != NULL);

Need to gracefully handle errors here? For example what happens if out of disk space.

@@ +252,3 @@
+		                         (const gchar *)data, n_data, error)) {
+			g_assert (error == NULL || *error != NULL);
+			return FALSE;

Leaking file_path.

@@ +260,3 @@
+			            g_file_error_from_errno(errno),
+			            _("Failed to set imported SSH key "
+			              "permissions to 600."));

Again, lets include the file name.

@@ +261,3 @@
+			            _("Failed to set imported SSH key "
+			              "permissions to 600."));
+			return FALSE;

Leaking file_path here.

@@ +267,3 @@
+		gcr_parsed_unref(g_queue_pop_head(self->pv->parseds));
+	}
+	g_free(ssh_dir);

Don't you need to run ssh-keygen as well to generate a public key?

@@ +282,3 @@
+	task = g_task_new(self, cancellable, callback, user_data);
+
+	g_task_run_in_thread(task, _gcr_openssh_importer_import_thread);

I don't think we should run in a thread. The tasks that are currently in _gcr_openssh_importer_import_sync() can all be run synchronously even when going async. The only thing that would probably need to be done differently, is running ssh-keygen. 

I would suggest making a function with common actions that both sync/async run (all the file writing stuff above), and then have the sync/async versions respectively run ssh-keygen synchronously and asynchronously .

::: gcr/gcr-openssh-importer.h
@@ +2,3 @@
+ * gnome-keyring
+ *
+ * Copyright (C) 2013 Tails

Is "Tails" really a legal entity?

@@ +24,3 @@
+#if !defined (__GCR_H_INSIDE__) && !defined (GCR_COMPILATION)
+#error "Only <gcr/gcr.h> can be included directly."
+#endif

I don't think this header will be public (ie: installed), so we don't need this.

::: gcr/gcr-parser.c
@@ -2591,3 @@
-		}
-		parsed = parsed->next;
-		if (parsed->data != NULL) {

Why do we need this change? I realize you want to keep the entire stack of parsed stuff around. But is that really necessary? Can't GcrOpensshImporter just ref the parsed blocks it needs when things are queued?

Review of attachment 253954 [details] [review]:

FWIW, the commit message sounds right.

Created attachment 255543 [details] [review]
Parser: relax const requirement for parse_data

Previously gcr_parser_parse_data took constant data to put into a
GBytes (which eventually winds up in GcrParseds). While, this *could*
be correct, it's often unreasonably cumbersome to actually provide
data that will forever be constant. Indeed, clients of this API often
pretended to provide const data -- a very dangerous thing.

This fragility manifested itself in a very odd bug seen when
implementing the OpenSshImporter: the data held by a Parsed's GBytes
(properly g_bytes_ref'd) managed to change from under me between calls
to create_for_parsed and parse()!

The problem was that gcr-viewer-widget got some "const" data from
gcr-unlock-renderer, which, in the renderer's finalize function was
freed, resulting in the aforementioned GBytes data inconsistency.

There isn't a good reason to make the GBytes rely on getting const
data from clients, when GBytes provides a nice alternate initializer
which copies the data.

This change switches the API requirements to be easier to meet by
copying the data into the GBytes.

Created attachment 255544 [details] [review]
WIP Add SSH Private Key Import (v3)

1. Whitespace/comment fixups
2. Don't leak memory when errors occur
3. Don't asynchronously import from a thread; create a shared function
between import_sync and import_async

(In reply to comment #11)
> Review of attachment 253953 [details] [review]:
> 
Thank you so much for the review -- it's really helpful.
> 
> ::: gcr/gcr-openssh-importer.c
> @@ +85,3 @@
> +calculate_uri (GcrOpensshImporter *self)
> +{
> +    return g_build_filename("file://", g_get_home_dir(), ".ssh", NULL);
> 
> The URI is just used as a unique identifier so an app can later import to the
> same place. Would recommend "x-openssh://" or something like that to indicate:
> the default location for openssh keys. When this becomes constant, then me
> would just remove this function all together.
I changed it to x-openssh:// though it's still not clear to me what it's used for (and perhaps it's not a big deal since it doesn't look like it's used anywhere). 
> 
> 
> @@ +247,3 @@
> +
> +        file_path = g_strconcat(ssh_dir, G_DIR_SEPARATOR_S,
> +                                "imported_id_rsa",
> 
> How does this deal with multiple keys being imported one after the other?
In terms of the filename, that's not implemented yet. I imagine that in the loop I'll look up the filename for each key.

> 
> @@ +251,3 @@
> +        if (!g_file_set_contents(file_path,
> +                                 (const gchar *)data, n_data, error)) {
> +            g_assert (error == NULL || *error != NULL);
> 
> Need to gracefully handle errors here? For example what happens if out of disk
> space.
In terms of error checking, the v3 patch below should clean up after itself now, if that's what you meant by gracefully handle errors. FWIW, this is very similar to the example provided here https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html

Specifically, I'm assuming that g_file_set_contents will do what it needs if there's an error, which will set the 'error' variable. My function will clean up memory, and pass 'error' to the caller, which will hopefully indicate to the user that something is wrong. Right?

> 

> 
> @@ +267,3 @@
> +        gcr_parsed_unref(g_queue_pop_head(self->pv->parseds));
> +    }
> +    g_free(ssh_dir);
> 
> Don't you need to run ssh-keygen as well to generate a public key?
Indeed. I haven't gotten to that yet. 
> 
> @@ +282,3 @@
> +    task = g_task_new(self, cancellable, callback, user_data);
> +
> +    g_task_run_in_thread(task, _gcr_openssh_importer_import_thread);
> 
> I don't think we should run in a thread. The tasks that are currently in
> _gcr_openssh_importer_import_sync() can all be run synchronously even when
> going async. The only thing that would probably need to be done differently, is
> running ssh-keygen. 
> 
> I would suggest making a function with common actions that both sync/async run
> (all the file writing stuff above), and then have the sync/async versions
> respectively run ssh-keygen synchronously and asynchronously .
> 
I created that common function in v3. As for running ssh-keygen asynchronously, I am under the assumption that I ought to just copy _gcr_gnupg_process_run_async(), right?
That function utilizes g_spawn_async_with_pipes(), but there doesn't seem to be a g_spawn_sync_with_pipes(), so I'm not sure how to run ssh-keygen synchronously. Any tips?

> 
> ::: gcr/gcr-parser.c
> @@ -2591,3 @@
> -        }
> -        parsed = parsed->next;
> -        if (parsed->data != NULL) {
> 
> Why do we need this change? I realize you want to keep the entire stack of
> parsed stuff around. But is that really necessary? Can't GcrOpensshImporter
> just ref the parsed blocks it needs when things are queued?
Unfortunately, the parsed passed to create_parsed (the first thing called in GcrOpensshImporter) has already been gcr_ref'd by some gtk library due to it being an argument in a signal or something. 
http://i.imgur.com/cZyp131.png


Also, once I imported in the same thread, nemiver stopped freezing, and I could finally track down that odd GBytes bug! So now, importing for rsa/dsa and encrypted/unencrypted works great! The only things left are implementing my plan for filenames, and figuring out how to run ssh-keygen :)

Created attachment 255625 [details] [review]
Parser: Don't use g_bytes_new_static in parse_data

Previously, gcr_parser_parse_data assumed that the const data it
received as an argument was static (never modified or freed), but
this was not always the case.

For example: gcr-viewer-widget gets const (but not static) data from
gcr-unlock-renderer, which is freed in the renderer's finalize function.
Any use of args.data after this point is erroneously dealing with freed
memory.

Remember: const != static

Created attachment 255626 [details] [review]
Viewer: Always unref fis

Above, g_file_read_finish returns something that should be unreffed
to free it. Previously, if this function didn't go into this else
block, fis was not unreffed. This commit makes fis unreffed all
the time.

Created attachment 255627 [details] [review]
WIP Add SSH Private Key Import (v4)

Properly free parseds queue

Created attachment 255628 [details] [review]
Parser: Store imported key filename in Parsed & Parser

1. when the GcrViewerWindow reads a key, and creates a parser, store
the filename as an instance variable of the parser
2. in the parse_...() functions where the GcrParsed is created, store
the filename as an instance variable via push_parsed (read it from
the parser).
3. Access to the filename is useful in many ways. For example,
importers, e.g. GcrOpenSshImporter, has access to the GcrParsed,
and can read the filename to preserve filenames of imported keys.

Created attachment 255629 [details] [review]
WIP - SshImporter: Preserve filename of imported key

Properly imports files with their filename, but doesn't yet check
for overwriting keys.

Created attachment 255637 [details] [review]
Parser: Store imported key filename in Parsed & Parser

1. when the GcrViewerWindow reads a key, and creates a parser, store
the filename as an instance variable of the parser
2. in the parse_...() functions where the GcrParsed is created, store
the filename as an instance variable via push_parsed (read it from
the parser).
3. Access to the filename is useful in many ways. For example,
importers, e.g. GcrOpenSshImporter, has access to the GcrParsed,
and can read the filename to preserve filenames of imported keys.

Review of attachment 255626 [details] [review]:

g_file_read_finish() only returns a non-NULL pointer when &error is not set. In addition we cannot pass NULL to g_object_unref().

Moved work on GcrParser to a new bug #708736. Hashem could you give a quick review of the code there?

Implemented the TODO in a patch there. Including the copying of the input data in gcr_parser_parse_data(), if caller does not provide a GBytes we can reference.

Review of attachment 255637 [details] [review]:

Thanks. This looks like a useful good addition. Just a few tweaks needed.

::: gcr/gcr-parser.c
@@ +128,3 @@
 	gboolean sensitive;
 	GcrDataFormat format;
+	const gchar *filename;

This should probably be a 'gchar *' and not a 'const gchar'.

@@ +137,3 @@
 	GPtrArray *passwords;
 	GcrParsed *parsed;
+	const gchar *filename;

Ditto.

@@ +397,3 @@
 		g_bytes_unref (parsed->data);
 	g_free (parsed->label);
+	g_free ((gchar *)parsed->filename);

The cast isn't needed when type is changed above.

@@ +2189,3 @@
 	self->pv->passwords = g_ptr_array_new ();
 	self->pv->normal_formats = TRUE;
+	self->pv->filename = NULL;

We don't need to initialize fields that are set to zero.

@@ +2570,3 @@
+ */
+const gchar*
+ *

For better or worse, we don't use const when referring to GObject pointers. They're always non-const.

@@ +2571,3 @@
+const gchar*
+gcr_parser_get_filename (const GcrParser *self)
+ *

Could you add a g_return_val_if_fail (GCR_IS_PARSER (self), NULL); here?

@@ +2584,3 @@
+void gcr_parser_set_filename (GcrParser *self, const gchar *filename)
+{
+	return self->pv->filename;

File names are not always UTF8. I don't think we need to have this check. In addition, it seems it should be possible to set the filename to NULL, no?

Could you add a g_return_if_fail (GCR_IS_PARSER (self)); check here?

@@ +2885,3 @@
+const gchar*
+gcr_parsed_get_filename (const GcrParsed *parsed)
+ * Get the filename of the parsed item.

Hmmm, we haven't been referring to GcrParsed with constness, as it's a boxed struct. Was there a reason that you thought it would make sense?

Could you do a check like g_return_val_if_fail (parsed != NULL, NULL); here?

(In reply to comment #24)
> Review of attachment 255637 [details] [review]:
> 
> Thanks. This looks like a useful good addition. Just a few tweaks needed.
> 
> ::: gcr/gcr-parser.c
> @@ +128,3 @@
>      gboolean sensitive;
>      GcrDataFormat format;
> +    const gchar *filename;
> 
> This should probably be a 'gchar *' and not a 'const gchar'.
> 
> @@ +137,3 @@
>      GPtrArray *passwords;
>      GcrParsed *parsed;
> +    const gchar *filename;
> 
> Ditto.
> 
> @@ +397,3 @@
>          g_bytes_unref (parsed->data);
>      g_free (parsed->label);
> +    g_free ((gchar *)parsed->filename);
> 
> The cast isn't needed when type is changed above.
My thinking in choosing const gchar * is that I want filename to be immutable. Const allows the compiler to ensure that, but the downside is that freeing requires a cast.

> @@ +2584,3 @@
> +void gcr_parser_set_filename (GcrParser *self, const gchar *filename)
> +{
> +    return self->pv->filename;
> 
> File names are not always UTF8. I don't think we need to have this check. In
> addition, it seems it should be possible to set the filename to NULL, no?
> 
in gcr-viewer-widget.c I have:

	basename = g_file_get_basename (file);
	display_name = g_filename_display_name (basename);
	gcr_parser_set_filename (self->pv->parser, display_name);

get_basename says it has "no particular encoding". I thought that might be dangerous. I figured it'd be best to enforce any particular encoding. Most functions here deal with glib file name encoding, but I don't see any functions to validate that encoding. Is it okay to do no checks or guarantees of encoding?

> 
> @@ +2885,3 @@
> +const gchar*
> +gcr_parsed_get_filename (const GcrParsed *parsed)
> + * Get the filename of the parsed item.
> 
> Hmmm, we haven't been referring to GcrParsed with constness, as it's a boxed
> struct. Was there a reason that you thought it would make sense?
> 
I read up on recommended usage of const: http://www.possibility.com/Cpp/const.html
get_filename ought not to change *parsed, so I made it const.

(In reply to comment #25)
> (In reply to comment #24)
> > @@ +397,3 @@
> >          g_bytes_unref (parsed->data);
> >      g_free (parsed->label);
> > +    g_free ((gchar *)parsed->filename);
> > 
> > The cast isn't needed when type is changed above.
> My thinking in choosing const gchar * is that I want filename to be immutable.
> Const allows the compiler to ensure that, but the downside is that freeing
> requires a cast.

We should return it from gcr_parsed_get_filename() as const, but hold it internally just as a non-const gchar* to reflect the fact that the string is owned here.

> > @@ +2584,3 @@
> > +void gcr_parser_set_filename (GcrParser *self, const gchar *filename)
> > +{
> > +    return self->pv->filename;
> > 
> > File names are not always UTF8. I don't think we need to have this check. In
> > addition, it seems it should be possible to set the filename to NULL, no?
> > 
> in gcr-viewer-widget.c I have:
> 
>     basename = g_file_get_basename (file);
>     display_name = g_filename_display_name (basename);
>     gcr_parser_set_filename (self->pv->parser, display_name);
> 
> get_basename says it has "no particular encoding". I thought that might be
> dangerous. I figured it'd be best to enforce any particular encoding. Most
> functions here deal with glib file name encoding, but I don't see any functions
> to validate that encoding. Is it okay to do no checks or guarantees of
> encoding?

Yes, it's fine to have no checks on the file name encoding at this point. Checking filename encoding is complex and surprising. The glib file read/write machinery gets this right, so lets leave checks until a caller uses the filename there.

> > @@ +2885,3 @@
> > +const gchar*
> > +gcr_parsed_get_filename (const GcrParsed *parsed)
> > + * Get the filename of the parsed item.
> > 
> > Hmmm, we haven't been referring to GcrParsed with constness, as it's a boxed
> > struct. Was there a reason that you thought it would make sense?
> > 
> I read up on recommended usage of const:
> http://www.possibility.com/Cpp/const.html
> get_filename ought not to change *parsed, so I made it const.

First of all that's a C++ guide, not about C. But in general the style for GObject or boxed struct based immutability is enforced by the object rather than the compiler. So lets leave this as non-const @parsed argument.

Created attachment 256233 [details] [review]
Parser: Store imported key filename in Parsed & Parser

1. when the GcrViewerWindow reads a key, and creates a parser, store
the filename as an instance variable of the parser
2. in the parse_...() functions where the GcrParsed is created, store
the filename as an instance variable via push_parsed (read it from
the parser).
3. Access to the filename is useful in many ways. For example,
importers, e.g. GcrOpenSshImporter, has access to the GcrParsed,
and can read the filename to preserve filenames of imported keys.

Review of attachment 256233 [details] [review]:

This patch looks good.

 * It doesn't apply to git master (anymore?), probably needs rebasing.
 * Could you write a test-parser.c test which exercises the functionality?
 * Is there anywhere we should be calling gcr_parser_set_filename()?

One last question. Do you think it makes sense to use a GFile here instead of a filename? That would help resolve some of the questions around file name encoding, and make sure we can represent all types of files appropriately.

Review of attachment 256233 [details] [review]:

This patch looks good.

 * It doesn't apply to git master (anymore?), probably needs rebasing.
 * Could you write a test-parser.c test which exercises the functionality?
 * Is there anywhere we should be calling gcr_parser_set_filename()?

One last question. Do you think it makes sense to use a GFile here instead of a filename? That would help resolve some of the questions around file name encoding, and make sure we can represent all types of files appropriately.

Created attachment 256444 [details] [review]
commit cherry-picked and manually format-patched

The patch previously didn't apply since it wasn't a child of master (it was just a context issue in the diff).

I changed the commit message to make it more clear.

Also, I don't think GFile is necessary since API looks like overkill for the purposes of the additional information we want to associate with a Parser/Parsed, namely a basename. 
The GFile docs say "One way to think of a GFile is as an abstraction of a pathname". We're not doing things with path names, and I don't think we want to since the import/export operations of gcr work on individual files in  isolation from where they are on the filesystem. 
Additionally, we don't (and I don't see why we would) need to do path manipulations, which is what GFile seems to be good for.

I did some research this morning on how to generate public keys (which seahorse needs before it will display an imported key).

We can generate a public key with:
ssh-keygen -y -f private_key_here -P "password_here"

I was worried we'd have to find a way to provide the password via some stdin hack, but thankfully the -P options works with -y (though it doens't mention it in the man page -- I had to look through the openssh source).

So now all I have to do is figure out how gcr-gnupg-process.c works. I wish gsubprocesses was merged!

...one step at a time...

(In reply to comment #31)
> I was worried we'd have to find a way to provide the password via some stdin
> hack, but thankfully the -P options work

Passing secrets on the command-line, really? Then they can be snooped in the list of processes.

(In reply to comment #32)
> (In reply to comment #31)
> > I was worried we'd have to find a way to provide the password via some stdin
> > hack, but thankfully the -P options work
> 
> Passing secrets on the command-line, really? Then they can be snooped in the
> list of processes.

Ah good point. Thanks for the feedback. I'll need to find a different workaround then.

In my experimentation I initially tried passing the password to ssh-keygen via stdin (with ssh-keygen -y -f private key <<< "password"), but ssh-keygen failed with "ssh_askpass: exec(/usr/libexec/openssh/ssh-askpass): No such file or directory
load failed"

Thoughts on http://stackoverflow.com/questions/1340366/how-to-make-ssh-receive-the-password-from-stdin ?

This looks doable.
http://andre.frimberger.de/index.php/linux/reading-ssh-password-from-stdin-the-openssh-5-6p1-compatible-way/

(In reply to comment #30)
> Created an attachment (id=256444) [details] [review]
> commit cherry-picked and manually format-patched
> 
> The patch previously didn't apply since it wasn't a child of master (it was
> just a context issue in the diff).
> 
> I changed the commit message to make it more clear.
> 
> Also, I don't think GFile is necessary since API looks like overkill for the
> purposes of the additional information we want to associate with a
> Parser/Parsed, namely a basename. 

Okay. Fair enough. In any case we can add a GFile later if necessary.

Thanks for the rebase. Made the following changes before pushing:

 * Added to gcr-base.symbols (in the future, please run 'make distcheck'
   to catch this)
 * Documentation for new API's need to go gcr-sections.txt (you can find
   them listed in gcr-unused.txt).
 * Fixed indentation and spacing in gcr_parser_set_filename()
 * Added test case to test-parser.c
 * Removed documentation referring to UTF-8

> This looks doable.
> http://andre.frimberger.de/index.php/linux/reading-ssh-password-from-stdin-the-openssh-5-6p1-compatible-way/

The main problem with that approach is writing the password to a temporary file. That's just a bad idea, as we should never write passwords directly to disk.

But that's how seahorse does it, except it prompts, which we can't do here:

https://git.gnome.org/browse/seahorse/tree/ssh/seahorse-ssh-operation.c#n335

Another way is to allocate a proper TTY and have SSH read from it:

https://git.gnome.org/browse/gvfs/tree/daemon/gvfsbackendsftp.c#n480

Another implementation of that same code is here, in sshpass:

http://sourceforge.net/p/sshpass/code/HEAD/tree/trunk/main.c#l195

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gcr/-/issues/68.